Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

[PAN-2302] Resume world state download from existing queue #1016

Merged
merged 15 commits into from
Mar 4, 2019

Conversation

mbaxter
Copy link
Contributor

@mbaxter mbaxter commented Feb 28, 2019

PR description

This PR updates RocksDbTaskQueue so that on construction, any existing data on disk will be read and accounted for. Previously, the queue would always be empty on initialization. Now that the queue can be resumed, the WorldStateDownloader modifies its behavior when started with a non-empty queue. Rather than requesting the root node when the download begins, this node will only be requested when all other nodes have been processed. This prevents us from queueing the root node multiple times each time the download is resumed.

Additionally, some trie node utility methods were made more generic and moved from the tests into TrieNodeDecoder, which lead to the deduplication of some logic in TrieNodeDataRequest.

Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

Just the one TODO that probably needs addressing or removing but otherwise LGTM.

assertThat(result.size()).isEqualTo(1);
assertThat(result.get(0).getValue()).contains(BytesValue.of(1));
// TODO: fix path representation
// assertThat(result.get(0).getPath()).isEqualTo(BytesValue.fromHexString("0x100000"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need looking at still?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - I'll make a ticket for this. It's an existing issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this isn't actually a "bug" - the path value is just handled in an idiosyncratic way. Updated the test.

@mbaxter mbaxter merged commit 3ac6f47 into PegaSysEng:master Mar 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants