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

improvement(header/p2p): parallelize GetRangeByHeight request #1206

Merged
merged 22 commits into from
Nov 28, 2022
Merged

improvement(header/p2p): parallelize GetRangeByHeight request #1206

merged 22 commits into from
Nov 28, 2022

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented Oct 7, 2022 •
edited
Loading

@vgonkivs vgonkivs self-assigned this Oct 7, 2022
@vgonkivs vgonkivs force-pushed the parallelize_getRange_request branch 4 times, most recently from 19b015a to 1ba8a5f Compare October 13, 2022 12:01
@vgonkivs vgonkivs changed the title Parallelize get range request improvement(header/p2p): parallelize GetRangeByHeight request Oct 13, 2022
@vgonkivs vgonkivs force-pushed the parallelize_getRange_request branch 2 times, most recently from 1545ddb to 4fd9684 Compare October 17, 2022 08:35
@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2022 •
edited
Loading

Codecov Report

Merging #1206 (2ea1649) into main (0849c19) will increase coverage by 0.28%.
The diff coverage is 76.72%.

@@            Coverage Diff             @@
##             main    #1206      +/-   ##
==========================================
+ Coverage   55.52%   55.81%   +0.28%     
==========================================
  Files         181      184       +3     
  Lines       11020    11316     +296     
==========================================
+ Hits         6119     6316     +197     
- Misses       4293     4380      +87     
- Partials      608      620      +12
Impacted Files Coverage Δ
header/p2p/peer_tracker.go 57.35% <57.35%> (ø)
header/p2p/session.go 73.97% <73.97%> (ø)
header/p2p/peer_stats.go 93.54% <93.54%> (ø)
header/p2p/exchange.go 66.86% <100.00%> (-2.32%) ⬇️
nodebuilder/header/constructors.go 79.16% <100.00%> (+7.73%) ⬆️
header/p2p/server.go 66.37% <0.00%> (-5.31%) ⬇️
share/eds/byzantine/bad_encoding.go 66.00% <0.00%> (-3.00%) ⬇️
share/eds/byzantine/pb/share.pb.go 32.88% <0.00%> (-1.97%) ⬇️
share/ipld/get.go 92.62% <0.00%> (+1.38%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@vgonkivs vgonkivs force-pushed the parallelize_getRange_request branch 10 times, most recently from 588cc4a to e22f7b1 Compare October 17, 2022 13:21
@renaynay
Copy link
Member

renaynay commented Oct 17, 2022 •
edited
Loading

@Wondertan , for context this PR makes TestFullReconstructFromLights flakey for what seems to be the same reason behind the solution you found to the fx upgrade that borked the tests (cf3637c).

There's still flakiness around header-sub (when a peer joins, when a peer is actually able to receive messages via subscription), etc. I'm not sure that it is actually resolved by that manual connection between a node on startup via InitStore before syncer.

@vgonkivs vgonkivs force-pushed the parallelize_getRange_request branch 6 times, most recently from 58afbb3 to 47bcd36 Compare October 25, 2022 13:08
distractedm1nd
distractedm1nd previously approved these changes Nov 25, 2022
View reviewed changes
Copy link
Collaborator

@distractedm1nd distractedm1nd left a comment

Choose a reason for hiding this comment

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

only reviewed last commit, from my understanding this deadlock was the last blocker, so now approving

Wondertan
Wondertan reviewed Nov 27, 2022
View reviewed changes
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Last comments. Everything else seems totally legit

header/p2p/session.go Show resolved Hide resolved
header/p2p/session.go Show resolved Hide resolved
header/p2p/session.go Show resolved Hide resolved
Wondertan
Wondertan approved these changes Nov 28, 2022
View reviewed changes
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

:shipit:

renaynay
renaynay approved these changes Nov 28, 2022
View reviewed changes
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

LFM
Let's finally merge :)

@vgonkivs vgonkivs merged commit b3d019d into celestiaorg:main Nov 28, 2022
@vgonkivs vgonkivs deleted the parallelize_getRange_request branch January 9, 2023 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@renaynay renaynay renaynay approved these changes

@Wondertan Wondertan Wondertan approved these changes

@walldiss walldiss Awaiting requested review from walldiss walldiss is a code owner

@distractedm1nd distractedm1nd Awaiting requested review from distractedm1nd

Assignees

@vgonkivs vgonkivs

Labels
area:header Extended header area:p2p
Projects
No open projects
Celestia Node
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vgonkivs @codecov-commenter @renaynay @walldiss @distractedm1nd @Wondertan