[PAN-2427] Prep chain downloader for branch by abstraction #1194
Conversation
| this.protocolContext = protocolContext; | ||
| this.syncState = syncState; | ||
|
|
||
| this.blockPropagationManager = |
There was a problem hiding this comment.
I think the terminology here is getting a bit fuzzy. I think the "FullSynchronizer" is really a "FullDownloaderFactory" (similarly "FastSynchronizer" -> "FastDownloaderFactory"). And I think that the blockPropagationManager should belong to the Synchronizer. In my mind, the synchronizer encapsulates all things related to staying up-to-date with the network: tracking peer stats, managing propagated blocks, downloading from the network. And full / fast modes are only concerned with how we download data from the network.
There was a problem hiding this comment.
Yep, I can see that working. Done.
…astSynchronizer and FullSynchronizer to FastDownloaderFactory and FullDownloaderFactory.
…wnloader-abstraction
|
|
||
| fastSynchronizer = | ||
| FastSynchronizer.create( | ||
| this.fastSynchronizer = |
There was a problem hiding this comment.
fastSynchronizer -> fastDownloaderFactory
| } | ||
|
|
||
| public void start() { | ||
| chainDownloader.start(); |
There was a problem hiding this comment.
To follow-through on the factory change, should this class now create() a Downloader that has a start() method? Just seems weird to call Factory.start().
There was a problem hiding this comment.
We can't really because there are additional methods the "factories" need to expose - trailing peers for full sync and deleting the state for fast.
There was a problem hiding this comment.
Maybe these are FastDownloadManager and FullDownloadManager? Manager is a bit of a weasel word but I'm not seeing a better layout that would give clearer responsibilities at the moment.
There was a problem hiding this comment.
I think you could get away with a Downloader that just exposes start and calculateTrailingPeerRequirements, then does any cleanup before the start future completes. But feel free to just rename if you think that makes more sense.
There was a problem hiding this comment.
I'm not sure we've got this structure quite right yet but I think I've found a reasonable landing point. Fast sync has a FastSyncDownloadFactory which returns a FastSyncDownload - full sync is much simpler so doesn't need the factory and just creates a FullSyncDownload directly.
End result is that DefaultSynchronizer winds up with a Optional<FastSyncDownload> and a FullSyncDownload both of which have start and calculateTrailingPeerRequirements.
There's more tidy-up that could be done in this area but I'm hesitant to get too distracted.
| } | ||
|
|
||
| public void start() { | ||
| chainDownloader.start(); |
There was a problem hiding this comment.
I think you could get away with a Downloader that just exposes start and calculateTrailingPeerRequirements, then does any cleanup before the start future completes. But feel free to just rename if you think that makes more sense.
…wnloaderFactory to FullSyncDownloader since it doesn't actually need a factory.
PR description
Refactor to make
ChainDownloaderan interface so that we can implement alternate chain download implementations. The primary benefit is allow us to use branch-by-abstraction to develop a pipeline based chain download in smaller steps rather than having to wholesale replace the chain download mechanism.Also makes the abstractions more consistent between fast and full sync by introducing
FullSynchronizerwhich bundles together the full sync chain download, block propagation manager and trailing peers config soDefaultSynchronizeris more clearly just switching between fast and full sync. It also let the full sync chain downloader adhere to theChainDownloadapi and not need to expose the trailing peer config as well.