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.

Sign up for GitHub

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

Jump to bottom

Clear correctly heal pending task before restart #3920

Merged
merged 9 commits into from
Jun 22, 2022
Merged

Clear correctly heal pending task before restart #3920

merged 9 commits into from
Jun 22, 2022

Conversation

matkt
Copy link
Contributor

@matkt matkt commented May 30, 2022 •
edited
Loading

Signed-off-by: Karim TAAM karim.t2am@gmail.com

PR description

Properly clean the pending tasks when changing the pivot block in order to avoid blocking the snapsynnc process

Fixed Issue(s)

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if
    updates are required.

Changelog

@matkt
fix heal
eb97fb4 
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
@matkt matkt force-pushed the feature/snapsync-heal-stalled branch from 842631e to eb97fb4 Compare June 6, 2022 08:23
@matkt matkt changed the title add scheduler for reload heal step Clear correctly heal pending task before restart Jun 6, 2022
matkt added 4 commits June 6, 2022 15:58
@matkt
Merge branch 'main' into feature/snapsync-heal-stalled
0e50212
@matkt
fix reload pivot block
028c0d9 
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
@matkt
fix nullointer
a31f394 
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
@matkt
update tests
9903954 
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
@matkt matkt marked this pull request as ready for review June 20, 2022 07:44
fab-10
fab-10 approved these changes Jun 20, 2022
View reviewed changes
Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

LGTM, please add a CHANGELOG entry

.../test/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/DynamicPivotBlockManagerTest.java Outdated
@@ -119,4 +119,32 @@ public void shouldSwitchToNewPivotBlockWhenNeeded() {
verify(snapSyncState).setCurrentHeader(pivotBlockHeader);
verify(fastSyncActions).waitForSuitablePeers(any());
}

@Test
public void shouldSwitchToNewPivotOnlyOnCe() {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/OnCe/Once/?

Copy link
Contributor

Choose a reason for hiding this comment

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

nah he meant Only on windows CE ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha. good catch

garyschulte
garyschulte reviewed Jun 20, 2022
View reviewed changes
.../src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/DynamicPivotBlockManager.java
@@ -93,9 +93,9 @@ public void switchToNewPivotBlock(final BiConsumer<BlockHeader, Boolean> onSwitc
LOG.info(
"Select new pivot block {} {}", blockHeader.getNumber(), blockHeader.getStateRoot());
syncState.setCurrentHeader(blockHeader);
lastPivotBlockFound = Optional.empty();
Copy link
Contributor

@garyschulte garyschulte Jun 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Since this is getting set in both check and switchToNewPivotBlock , perhaps we should use a final AtomicReference<BlockHeader> instead of a mutable Optional<BlockHeader>

IDK if the snap pipeline might be calling these concurrently, but it seems possible.

any place where we would want to have Optional.empty(), we can just set the atomic reference to null, and when accessing we can wrap with Optional.ofNullable(listPivotBlockFound.get())

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 tried to find out if it was a problem but I don't think so. we could change it, but I'm afraid it might slow down the pipeline with additional synchronization.

if you find a case that could cause a crash or something, don't hesitate, I may have missed it. but from what I see. there is no risk. even if the value changes the behavior seems to remain valid

@matkt
fix review
ef58e0e 
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
@matkt matkt self-assigned this Jun 21, 2022
@fab-10
Copy link
Contributor

fab-10 commented Jun 22, 2022

please add a CHANGELOG entry

@matkt matkt enabled auto-merge (squash) June 22, 2022 14:25
@matkt matkt merged commit 5ee9be8 into hyperledger:main Jun 22, 2022
garyschulte pushed a commit that referenced this pull request Jul 7, 2022
@matkt @garyschulte
Snapsync clear correctly heal pending task before restart (#3920)
2d25800 
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
garyschulte added a commit that referenced this pull request Jul 7, 2022
@garyschulte
cherry-pick release of 22.4.4
219385e 
49e4fd8 -> worldstate not avail (#4069)
6aa8812 -> stateroot mismatch (#4041)
043191a -> jwt auth on websockets (#4039)
90f891b -> do not move head on exec and propose (#4013)
3baa4da -> upgrade for websockets (#4019)
5024c07 -> sepolia TTD (#4024)
5ee9be8 -> heal step in snap (#3920)
261b1e0 -> remove peer block height requirements (#3911)

Signed-off-by: garyschulte <garyschulte@gmail.com>
garyschulte added a commit that referenced this pull request Jul 7, 2022
@garyschulte
cherry-pick release of 22.4.4
649aff1 
49e4fd8 -> worldstate not avail (#4069)
6aa8812 -> stateroot mismatch (#4041)
043191a -> jwt auth on websockets (#4039)
90f891b -> do not move head on exec and propose (#4013)
b5fa62c -> sync check before processing remote transactions (4035)
3baa4da -> upgrade for websockets (#4019)
5024c07 -> sepolia TTD (#4024)
5ee9be8 -> heal step in snap (#3920)
261b1e0 -> remove peer block height requirements (#3911)

Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte garyschulte mentioned this pull request Jul 8, 2022
Merged
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
@matkt @eum602
Snapsync clear correctly heal pending task before restart (hyperledge…
baac329 
…r#3920)

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garyschulte garyschulte garyschulte left review comments

@fab-10 fab-10 fab-10 approved these changes

Assignees

@matkt matkt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@matkt @fab-10 @garyschulte