Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

netsync: Split blockmanager into separate package. #2500

Merged
merged 6 commits into from
Dec 15, 2020

Conversation

davecgh
Copy link
Member

@davecgh davecgh commented Dec 10, 2020

This requires #2498 and #2499.

This does the minimum work necessary to refactor the block manager into its own internal package named netsync. The main motivation is that separating this code into its own package will improve its testability and more cleanly split the details related to syncing with the network from consensus validation.

The API will certainly need some additional cleanup and changes to make it more usable outside of the specific circumstances it currently supports, however it is better to do that in future commits in order to keep the changeset as small as possible during this refactor.

It has been carefully crafted into a series of individual commits that make each step easier to review as it would be a giant blob of changes that would be very hard to review otherwise. As a case in point, a temporary interface is introduced and then removed later to allow renaming to happen in a separate commit while ensuring every individual commit continues to build and work properly along the way.

Overview of the major changes:

  • Rename server blockManager field to syncManager
  • Rename BMGR logging subsystem to SYNC
  • Update logging to use the new subsystem
  • Create the new netsync package
  • Add package logger
  • Tie new package logger to SYNC subsystem logger
  • Move blockmanager.go -> internal/netsync/manager.go
  • Rename blockManagerConfig to Config (now netsync.Config)
  • Move peerNotifier to netsync/interface.go (now netsync.PeerNotifier)
  • Rename newBlockManager to New (now netsync.New)
  • Rename blockManager to SyncManager (now netsync.SyncManager)
  • Update all references to the block manager to use the package
  • Add a skeleton README.md
  • Add a skeleton doc.go

This is work towards #1145.

@davecgh davecgh added this to the 1.7.0 milestone Dec 10, 2020
@davecgh davecgh mentioned this pull request Dec 10, 2020
28 tasks
@davecgh davecgh force-pushed the netsync_introduce_package branch 2 times, most recently from ba524ab to 9f2fca3 Compare December 11, 2020 06:10
Copy link
Member

@matheusd matheusd left a comment

Choose a reason for hiding this comment

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

Tested all commits with run_tests.go

Copy link
Member

@dnldd dnldd left a comment

Choose a reason for hiding this comment

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

Should be good to go after a rebase.

Copy link
Member

@rstaudt2 rstaudt2 left a comment

Choose a reason for hiding this comment

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

Looks great. The new code is definitely clearer in terms of what SyncManager handles.

One small note, I noticed that there are still a few leftover comments referencing blockManager in internal/netsync/manager.go and some comments in server.go referring to blockmanager as well.

This renames the field that houses the block manager to syncManager to
more accurately reflect its purpose and updates comments accordingly.

This is part of an overall effort to split the block manager out into a
separate internal netsync package.
This renames the block manager logging subsystem from BMGR to SYNC and
updates all related documentation accordingly.

This is part of an overall effort to split the block manager out into a
separate internal netsync package.
This adds a temporary sync manager interface to facilitate cleaner diffs
when moving the block manager to a new package.

This is part of an overall effort to split the block manager out into a
separate internal netsync package.
This does the minimum work necessary to refactor the block manager into
its own internal package named netsync.  The main motivation is that
separating this code into its own package will improve its testability
and more cleanly split the details related to syncing with the network
from consensus validation.

The API will certainly need some additional cleanup and changes to make
it more usable outside of the specific circumstances it currently
supports, however it is better to do that in future commits in order to
keep the changeset as small as possible during this refactor.

Overview of the major changes:

- Create the new package
- Move blockmanager.go -> internal/netsync/manager.go
- Update logging to use the new netsync package logger
- Rename blockManagerConfig to Config (now netsync.Config)
- Move peerNotifier to netsync/interface.go (now netsync.PeerNotifier)
- Move syncManager to netsync/interface.go (now netsync.SyncManager)
- Rename newBlockManager to New (now netsync.New)
- Update all references to the block manager to use the package
- Add package logger
- Initialize new package logger with sync logger
This removes the temporary netsync.SyncManager interface that was added
to facilitate refactoring the block manager to the new netsync package
and renames the blockManager struct to SyncManager so it is exported.

It also updates callers to use the concrete struct versus the removed
interface.
@davecgh
Copy link
Member Author

davecgh commented Dec 15, 2020

@rstaudt2 Thanks for pointing out the missed comments. Looks like I missed some in the shuffle when breaking this all up into more easily digestible commits for review. I've modified the respective commits to update all of the relevant comments and log messages.

Note that I did not modify any mining since I noticed that it still has what appears to be leftover cruft, in the form of the unexported blockManagerFacade, from when it was split into an internal package. I'll take care of cleaning that bit up in another PR.

@davecgh davecgh merged commit e7a3670 into decred:master Dec 15, 2020
@davecgh davecgh deleted the netsync_introduce_package branch December 15, 2020 21:09
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.

None yet

4 participants