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

refactor(ipld): rework Retriever #738

Merged
merged 10 commits into from
Jun 3, 2022
Merged

refactor(ipld): rework Retriever #738

merged 10 commits into from
Jun 3, 2022

Conversation

Wondertan
Copy link
Member

@Wondertan Wondertan commented May 20, 2022 •
edited
Loading

  • Retrieve is now blocking forever until either context is canceled or square is reconstructed
  • Quadrant requesting is now async and detached from reconstruction attempts
  • session provides a done channel that notifies users if there enough shares were downloaded to try reconstruction

Tested with #702
Closes #706

@Wondertan Wondertan self-assigned this May 20, 2022
@Wondertan Wondertan requested review from liamsi and renaynay as code owners May 20, 2022 18:09
@Wondertan Wondertan force-pushed the hlib/retriever-rework-timeouting branch 2 times, most recently from 9911ede to 18fe92a Compare May 20, 2022 19:03
liamsi
liamsi reviewed May 21, 2022
View reviewed changes
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

First pass. Look'n good 👍🏼

ipld/retriever.go Outdated Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
@renaynay
Copy link
Member

Tests failing

@Wondertan Wondertan force-pushed the hlib/retriever-rework-timeouting branch 2 times, most recently from eb46b97 to 4a9103a Compare May 24, 2022 16:27
@Wondertan
Copy link
Member Author

Oh yeah, that's another flakey one which his now fixed

@Wondertan Wondertan requested a review from vgonkivs May 24, 2022 16:28
@Wondertan Wondertan force-pushed the hlib/retriever-rework-timeouting branch from 03b12da to d0da691 Compare May 24, 2022 16:38
@Wondertan Wondertan requested a review from liamsi May 24, 2022 16:42
@Wondertan Wondertan force-pushed the hlib/retriever-rework-timeouting branch from d0da691 to a96aa2c Compare May 24, 2022 16:47
@Wondertan
Copy link
Member Author

Ok, goroutines limit again... aaaa

@Wondertan Wondertan force-pushed the hlib/retriever-rework-timeouting branch from a96aa2c to 938cfca Compare May 25, 2022 12:07
@codecov-commenter
Copy link

codecov-commenter commented May 25, 2022 •
edited
Loading

Codecov Report

Merging #738 (b885208) into main (11a4bc9) will increase coverage by 0.25%.
The diff coverage is 94.80%.

@@            Coverage Diff             @@
##             main     #738      +/-   ##
==========================================
+ Coverage   52.46%   52.72%   +0.25%     
==========================================
  Files         116      116              
  Lines        6612     6669      +57     
==========================================
+ Hits         3469     3516      +47     
- Misses       2782     2795      +13     
+ Partials      361      358       -3
Impacted Files Coverage Δ
ipld/get_shares.go 81.63% <ø> (+8.16%) ⬆️
ipld/retriever_quadrant.go 100.00% <ø> (ø)
ipld/retriever.go 88.63% <94.80%> (-1.48%) ⬇️
cmd/flags_misc.go 54.38% <0.00%> (-8.78%) ⬇️
node/settings.go 39.02% <0.00%> (-4.22%) ⬇️
node/services/config.go 46.66% <0.00%> (-3.34%) ⬇️
das/daser.go 73.68% <0.00%> (-2.64%) ⬇️
logs/logs.go 100.00% <0.00%> (ø)
node/node.go 62.50% <0.00%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11a4bc9...b885208. Read the comment docs.

@Wondertan
Copy link
Member Author

@liamsi, mind to re-revouz?

renaynay
renaynay reviewed May 30, 2022
View reviewed changes
ipld/get_shares.go Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
ipld/retriever.go Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
ipld/retriever.go Show resolved Hide resolved
ipld/retriever_test.go Outdated Show resolved Hide resolved
Wondertan added 3 commits May 31, 2022 19:38
@Wondertan
refactor(ipld): rework Retriever
b75f430 
* Retrieve is now blocking forever until either context is canceled or square is reconstructed
* Quadrant requesting is now async and detached from recontruction attempts
* session provides a done channel which notifies users if there enought shares were downloaded to try reconstruction
@Wondertan
docs(ipld): add todo
1be4a1e
@Wondertan
tests(share): fix test by setting context timeout
a211f97
@Wondertan Wondertan force-pushed the hlib/retriever-rework-timeouting branch from 938cfca to 425fa23 Compare May 31, 2022 17:44
@Wondertan
Copy link
Member Author

Wondertan commented May 31, 2022 •
edited
Loading

Rebase on main

@Wondertan @liamsi
fix(ipldl): Apply suggestions from @liamsi's code review
0422368 
Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>
@Wondertan Wondertan force-pushed the hlib/retriever-rework-timeouting branch from 425fa23 to 0422368 Compare June 1, 2022 14:47
@Wondertan
Copy link
Member Author

The goroutine limit issue in tests is now fixed.

@Wondertan Wondertan force-pushed the hlib/retriever-rework-timeouting branch from a1d54bf to 668f0b2 Compare June 1, 2022 15:08
@Wondertan
Copy link
Member Author

The failed test is #787 and I won't fix this in this PR.

renaynay
renaynay approved these changes Jun 2, 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.

lgtm

@vgonkivs
Copy link
Member

vgonkivs commented Jun 2, 2022

Looks good 🚀

adlerjohn
adlerjohn reviewed Jun 2, 2022
View reviewed changes
ipld/retriever.go Outdated Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
@Wondertan Wondertan merged commit fcb2600 into main Jun 3, 2022
@Wondertan Wondertan deleted the hlib/retriever-rework-timeouting branch June 3, 2022 12:04
@Wondertan Wondertan mentioned this pull request Jun 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liamsi liamsi liamsi approved these changes

@adlerjohn adlerjohn adlerjohn left review comments

@renaynay renaynay renaynay approved these changes

@vgonkivs vgonkivs vgonkivs approved these changes

Assignees

@Wondertan Wondertan

Labels
area:ipld IPLD plugin
Projects
No open projects
Celestia Node
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ipld: rework timeouting for Retriever
6 participants
@Wondertan @renaynay @codecov-commenter @vgonkivs @adlerjohn @liamsi