Skip to content

Remove fullSync command#21

Merged
rmulhol merged 3 commits intostagingfrom
remove-full-sync
Jan 13, 2020
Merged

Remove fullSync command#21
rmulhol merged 3 commits intostagingfrom
remove-full-sync

Conversation

@rmulhol
Copy link
Copy Markdown

@rmulhol rmulhol commented Dec 27, 2019

  • remove full sync command
  • remove full sync mode from contract watcher
  • remove code only used by either of the above

Thinking this code should be removed since we don't recommend using it and aren't actively maintaining it. Hoping that clearing out the clutter will make the codebase more nimble and assist with creating more helpful documentation.

  • remove "HeaderSync" qualifier in var/struct/migration names

@rmulhol rmulhol force-pushed the remove-full-sync branch 2 times, most recently from a2959e9 to 7d14926 Compare December 28, 2019 04:29
@m0ar
Copy link
Copy Markdown

m0ar commented Jan 7, 2020

TODO still relevant or is this ready for review @rmulhol ?`

@rmulhol
Copy link
Copy Markdown
Author

rmulhol commented Jan 7, 2020

TODO still relevant or is this ready for review @rmulhol ?`

Still relevant but I'll take care of it soon

Rob Mulholand added 3 commits January 8, 2020 13:14
- not very practical to run and not maintained, removing to reduce
  clutter
- also removes other unused code
- No longer need multiple versions of core data structures without
  full sync version
Copy link
Copy Markdown

@yaoandrew yaoandrew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow.. great job. It looks so much cleaner when we remove some of this cruft and clean up the namespace.

This is accomplished through the use of the `headerSync`, `fullSync`, or `coldImport` commands.
These commands are described in detail [here](documentation/data-syncing.md).
This is accomplished through the use of the `headerSync` command.
This command is described in detail [here](documentation/data-syncing.md).
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

Expect(len(logs)).To(Equal(0))
})
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More of a general question, are these deleted test cases only needed for fullsync?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep - removing GetFullSyncLogs

Copy link
Copy Markdown

@elizabethengelman elizabethengelman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, this cleans things up a lot! 🌟

@@ -1,19 +0,0 @@
-- +goose Up
CREATE TABLE public.uncles (
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this table is no longer needed because we would only even sync an uncle through the fullSync, is that right?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep!

@rmulhol rmulhol merged commit b2dcd15 into staging Jan 13, 2020
@rmulhol rmulhol deleted the remove-full-sync branch January 13, 2020 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants