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

Pipeline chain download - fetch and import data #1207

Merged
merged 18 commits into from
Apr 4, 2019

Conversation

ajsutton
Copy link
Contributor

@ajsutton ajsutton commented Apr 3, 2019

PR description

Implements the pipeline steps to download headers, bodies and transaction receipts then fast import blocks.

Note that the pipeline is currently using thenProcessAsync which will need to be replaced with thenProcessAsyncPreservingOrder once it's available.

Toggle is still set to use the old chain download process.

@@ -189,7 +189,7 @@ public PipelineBuilder(
pipeEnd,
maximumBatchSize,
outputCounter.labels(lastStageName + "_outputPipe", "batches")),
bufferSize / maximumBatchSize + 1,
(int) Math.ceil(((double) bufferSize) / maximumBatchSize),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes the calculation so that a pipeline with buffer size 800, creating batches of 200 will hold 4 batches in the buffers (max 800 items in total). The previous integer calculation would wind up buffering up to 5 batches.

Copy link
Contributor

@mbaxter mbaxter left a comment

Choose a reason for hiding this comment

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

One small issue with getPendingFuturesCount but otherwise looks good!

@@ -59,4 +59,8 @@ public String toString() {
.add("end", end.getNumber())
.toString();
}

public int getSegmentLength() {
return (int) (end.getNumber() - start.getNumber());
Copy link
Contributor

Choose a reason for hiding this comment

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

As a general rule for cases like this, should we use Math.toIntExact or just use the simpler casting since we're not expecting large numbers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case is safe because the gap between checkpoint headers is originally specified as an int but having the explicit check probably makes sense anyway. So I've changed it to use toIntExact.

@@ -46,6 +46,10 @@ public void runPendingFutures() {
currentTasks.forEach(ExecutorTask::run);
}

public int getPendingFuturesCount() {
return tasks.size();
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to filter this by ExecutorTask::isPending or change the method name to countFutures

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -59,4 +59,8 @@ public String toString() {
.add("end", end.getNumber())
.toString();
}

public int getSegmentLength() {
Copy link
Contributor

Choose a reason for hiding this comment

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

(optional) If you change this to getSegmentLengthExclusive you can avoid the - 1 below: https://github.com/PegaSysEng/pantheon/pull/1207/files#diff-e42d52272bdde3b38192060ce222bebbR66

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tempting but it's only exclusive of one end, not both so could wind up being confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you -1, its exclusive of both ends :) [0,1] : 1 - 0 - 1 = 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️ I shouldn't try to do math so early in the morning...

@ajsutton ajsutton merged commit db4f60c into PegaSysEng:master Apr 4, 2019
@ajsutton ajsutton deleted the pipeline-download-fetch-data branch April 4, 2019 20:59
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