Separate download state tracking from WorldStateDownloader #967
Conversation
…iled requests from a stalled download don't wind up being added to the queue for a later attempt.
…old versions of DownloadState fields.
…n via the future.
|
|
||
| public abstract class NodeDataRequest { | ||
| private static final Logger LOG = LogManager.getLogger(); |
There was a problem hiding this comment.
I don't see LOG used, can this file be taken out of the PR?
| }); | ||
| } | ||
|
|
||
| private synchronized void doCancel(final Void result, final Throwable error) { |
There was a problem hiding this comment.
doShutdown? doCancel implies a failure state to me instead of a finished state.
| && !waitingForNewPeer; | ||
| } | ||
|
|
||
| public CompletableFuture<Void> getFuture() { |
There was a problem hiding this comment.
either the method name or the field name should be renamed. I would have expected to get the internal future instead of tearDownFuture or I would have expected to call getTearDownFuture - a field rename would involve both fields being renamed. future -> downloadingFuture and getFuture -> getTearDownFuture?
There was a problem hiding this comment.
Yeah that did wind up weird...
| */ | ||
| package tech.pegasys.pantheon.ethereum.eth.sync.worldstate; | ||
|
|
||
| enum WorldStateDownloadStatus { |
There was a problem hiding this comment.
Another unused file? Looks to have initially been factored out of WorldStateDownloader but it is no longer needed?
| assertThat(pendingRequests.isEmpty()).isTrue(); | ||
| } | ||
|
|
||
| private Runnable mustNotBeCalled() { |
There was a problem hiding this comment.
Putting on my paranoid tin foil hat, do we have a test where mustNotBeCalled is called so we can assert the failure escapes the lambdas? Or maybe I'm being too paranoid.
There was a problem hiding this comment.
I was going to say you were paranoid but Meredith had the same question. I know it works because I always check my tests fail but worth adding an explicit test.
|
|
||
| @Test | ||
| public void shouldStoreRootNodeBeforeReturnedFutureCompletes() { | ||
| future.thenAccept( |
There was a problem hiding this comment.
You need to check the result of this future - throwing inside the accept lambda won't trigger a failure.
| */ | ||
| package tech.pegasys.pantheon.ethereum.eth.sync.worldstate; | ||
|
|
||
| enum WorldStateDownloadStatus { |
There was a problem hiding this comment.
Looks like this is unused now
| } | ||
| } | ||
|
|
||
| public void whileAdditionalRequestCanBeSent(final Runnable action) { |
There was a problem hiding this comment.
| public void whileAdditionalRequestCanBeSent(final Runnable action) { | |
| public void whileAdditionalRequestsCanBeSent(final Runnable action) { |
| }); | ||
| } | ||
|
|
||
| private synchronized void doCancel(final Void result, final Throwable error) { |
There was a problem hiding this comment.
Maybe rename this to onShutdown or cleanup or something since it always runs?
There was a problem hiding this comment.
Yep, went with cleanup.
|
|
||
| public synchronized void markAsFailed(final Task<NodeDataRequest> task) { | ||
| if (!future.isDone()) { | ||
| task.markFailed(); |
There was a problem hiding this comment.
just an fyi - if task.markFailed() is called after the queue is cleared, it doesn't requeue the failed task.
There was a problem hiding this comment.
hmm, that's true. I know I had a test failing when I added it but it definitely seems fine now and all the code checks out. It's much nicer without it so removing...
PR description
The logic around tracking the state for a world state downloader is complex and hard to clearly analyse because it was mixed in with the details of performing actions in WorldStateDownloader. This PR splits the state tracking out to a separate class so it can be tested separately and the synchronization is clearer and more consistent.
Each download uses a new instance of of the state class which ensures that any events which occurs after the download is cancelled and restarted still see the cancelled future and abort correctly.
This revealed and fixes a couple of issues:
WaitForNewPeertask. Only one thread should do that to limit the number of timers required.