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.

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

feat!(header/p2p): add deadlines on stream and rework proto #1038

Merged
merged 8 commits into from
Oct 10, 2022

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented Aug 25, 2022

header/p2p/server.go Show resolved Hide resolved
header/p2p/server.go Show resolved Hide resolved
header/p2p/server.go Outdated Show resolved Hide resolved
@vgonkivs vgonkivs requested a review from renaynay August 29, 2022 10:34
@codecov-commenter
Copy link

codecov-commenter commented Aug 29, 2022

Codecov Report

Merging #1038 (fec03ad) into main (661e61b) will increase coverage by 0.18%.
The diff coverage is 73.78%.

@@            Coverage Diff             @@
##             main    #1038      +/-   ##
==========================================
+ Coverage   55.52%   55.70%   +0.18%     
==========================================
  Files         161      161              
  Lines        9582     9652      +70     
==========================================
+ Hits         5320     5377      +57     
- Misses       3734     3744      +10     
- Partials      528      531       +3     
Impacted Files Coverage Δ
header/p2p/server.go 53.70% <69.56%> (+0.44%) ⬆️
header/p2p/pb/extended_header_request.pb.go 40.00% <74.24%> (+7.13%) ⬆️
header/p2p/exchange.go 63.00% <78.57%> (+0.77%) ⬆️
ipld/get_namespaced_shares.go 90.51% <0.00%> (+2.18%) ⬆️

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

@Wondertan Wondertan added the kind:break! Attached to breaking PRs label Aug 29, 2022
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.

Note that this is protocol breaking, because of PB changes breaking backward compatibility. Thus the protocol ID/version has to be changed

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.

We also should not merge this in main before the upcoming release. Let's keep multiple breaking changes for HeaderEx protocol together and not spread those over multiple releases to avoid joint headache

header/p2p/server.go Show resolved Hide resolved
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.

Is the underlying impl of stream here yamux? If it so, then

// SetReadDeadline sets the deadline for future Read calls.

header/p2p/exchange.go Outdated Show resolved Hide resolved
header/p2p/exchange.go Outdated Show resolved Hide resolved
header/p2p/server.go Outdated Show resolved Hide resolved
@renaynay renaynay closed this Sep 6, 2022
@renaynay renaynay reopened this Sep 6, 2022
renaynay
renaynay previously approved these changes Sep 12, 2022
header/p2p/exchange.go Outdated Show resolved Hide resolved
header/p2p/server.go Outdated Show resolved Hide resolved
Wondertan
Wondertan previously approved these changes Sep 15, 2022
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.

The commit and the PR name should have an exclamation mark pointing to this PR as breaking. Other then that, lGTM

@vgonkivs vgonkivs changed the title header: add deadlines on stream and rework proto breaking(header): add deadlines on stream and rework proto Sep 19, 2022
@vgonkivs vgonkivs dismissed stale reviews from Wondertan and renaynay via d549ec5 September 19, 2022 10:34
@vgonkivs vgonkivs requested a review from walldiss September 19, 2022 10:48
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.

Please increment exchange protocol version

renaynay
renaynay previously approved these changes Sep 29, 2022
distractedm1nd
distractedm1nd previously approved these changes Sep 29, 2022
header/p2p/exchange.go Show resolved Hide resolved
walldiss
walldiss previously approved these changes Sep 30, 2022
Wondertan
Wondertan previously approved these changes Sep 30, 2022
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.

All tests passing besides integration (which is expected).

@vgonkivs vgonkivs merged commit 18860df into celestiaorg:main Oct 10, 2022
@vgonkivs vgonkivs deleted the headerex_improvement branch January 9, 2023 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:header Extended header kind:break! Attached to breaking PRs
Projects
No open projects
Status: Done
6 participants