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

feat(share): Implement ODSreader #1377

Merged
merged 11 commits into from
Nov 28, 2022
Merged

feat(share): Implement ODSreader #1377

merged 11 commits into from
Nov 28, 2022

Conversation

walldiss
Copy link
Member

@walldiss walldiss commented Nov 16, 2022 •
edited
Loading

Based on #1232,

The diff with original base PR is ods.go and ods_test.go.

@walldiss walldiss added area:shares Shares and samples kind:feat Attached to feature PRs labels Nov 16, 2022
@walldiss walldiss self-assigned this Nov 16, 2022
@MSevey
Copy link
Member

MSevey commented Nov 16, 2022

Why did none of the CI run on this PR?

MSevey
MSevey reviewed Nov 16, 2022
View reviewed changes
share/eds/blockstore.go Outdated Show resolved Hide resolved
MSevey
MSevey reviewed Nov 16, 2022
View reviewed changes
share/eds/blockstore.go Outdated Show resolved Hide resolved
MSevey
MSevey reviewed Nov 16, 2022
View reviewed changes
share/eds/blockstore.go Outdated Show resolved Hide resolved
MSevey
MSevey reviewed Nov 16, 2022
View reviewed changes
share/eds/blockstore.go Outdated Show resolved Hide resolved
MSevey
MSevey reviewed Nov 16, 2022
View reviewed changes
share/eds/store.go Outdated Show resolved Hide resolved
MSevey
MSevey reviewed Nov 16, 2022
View reviewed changes
share/eds/store_test.go Outdated Show resolved Hide resolved
MSevey
MSevey reviewed Nov 16, 2022
View reviewed changes
share/eds/ods.go Outdated Show resolved Hide resolved
MSevey
MSevey reviewed Nov 16, 2022
View reviewed changes
share/eds/store_test.go Outdated Show resolved Hide resolved
@Wondertan
Copy link
Member

Wondertan commented Nov 17, 2022 •
edited
Loading

@MSevey, most of the comments should go to #1232 as this PR is based on it

@distractedm1nd distractedm1nd linked an issue Nov 17, 2022 that may be closed by this pull request
feat(share/eds): ODSReader #1352
Closed
@MSevey
Copy link
Member

MSevey commented Nov 17, 2022

@walldiss if you create a PR that is based on another PR, it is helpful to target that PRs branch so the diff is only what you've done.

otherwise you should leave the PR in draft until it is ready for review.

@walldiss
Copy link
Member Author

@MSevey Is it possible to change target from fork branch to main repo later without recreating PR? At least right now I can't set target to fork, only main repo branches. We briefly discussed the stacking PR issue with @distractedm1nd and thought it would be nice to have feature branches for epics.

@distractedm1nd
Copy link
Collaborator

@MSevey the issue is that the branches are not merged yet, so they still exist on our forks. It also means that when we merge, it gets merged into a branch that needs to again be merged into main. Any good solutions? There is a lot of tooling around stacked PRs but github makes them kind of annoying to review

@MSevey
Copy link
Member

MSevey commented Nov 17, 2022

@MSevey the issue is that the branches are not merged yet, so they still exist on our forks. It also means that when we merge, it gets merged into a branch that needs to again be merged into main. Any good solutions? There is a lot of tooling around stacked PRs but github makes them kind of annoying to review

Do less stacked PRs 😜

@MSevey
Copy link
Member

MSevey commented Nov 17, 2022

@MSevey Is it possible to change target from fork branch to main repo later without recreating PR? At least right now I can't set target to fork, only main repo branches. We briefly discussed the stacking PR issue with @distractedm1nd and thought it would be nice to have feature branches for epics.

you can change the target branch by clicking edit, but as Ryan mentioned, it only works for branches in the main repo, you can't retarget to forked branches.

so it is better to just put this in draft until call the dependant code is merged.

Screen Shot 2022-11-17 at 10 14 12 AM

renaynay
renaynay reviewed Nov 18, 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.

Awesome :) and great test.

share/eds/ods.go Outdated Show resolved Hide resolved
share/eds/ods.go Outdated Show resolved Hide resolved
share/eds/ods.go Outdated Show resolved Hide resolved
share/eds/ods.go Outdated Show resolved Hide resolved
share/eds/ods.go Outdated Show resolved Hide resolved
share/eds/ods.go Outdated Show resolved Hide resolved
@walldiss
Copy link
Member Author

I've updated logic to greatly reduce amount of allocation by skipping extra slice allocation on every lead read. It is a bit harder to read but should be multiple times more performant now.

distractedm1nd
distractedm1nd requested changes Nov 22, 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.

excited for this

share/eds/ods.go Outdated Show resolved Hide resolved
share/eds/ods.go Show resolved Hide resolved
share/eds/ods.go Outdated Show resolved Hide resolved
share/eds/ods.go Outdated Show resolved Hide resolved
renaynay
renaynay previously approved these changes Nov 25, 2022
View reviewed changes
share/eds/ods.go Outdated Show resolved Hide resolved
distractedm1nd
distractedm1nd previously approved these changes Nov 25, 2022
View reviewed changes
Wondertan
Wondertan requested 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.

Super clean. Was a joy to read the code and the test! Requesting changes due to ODSReader signature and some other nits

share/eds/ods.go Outdated Show resolved Hide resolved
share/eds/ods.go Outdated Show resolved Hide resolved
share/eds/ods.go Outdated Show resolved Hide resolved
share/eds/ods.go Show resolved Hide resolved
share/eds/ods_test.go Show resolved Hide resolved
share/eds/ods.go Outdated Show resolved Hide resolved
Wondertan
Wondertan previously 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.

Good job!

vgonkivs
vgonkivs previously approved these changes Nov 28, 2022
View reviewed changes
walldiss and others added 10 commits November 28, 2022 14:15
@walldiss
add ODSReader
4616d74
@walldiss
add test comments
a119491
@walldiss @renaynay
Apply suggestions from code review
35af555 
Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>
@walldiss
Greatly reduce amount of buffer allocations
edc79af 
Apply review suggestions:
 - rescale Intn range
 - small renames & refactoring
@walldiss @distractedm1nd
Apply suggestions from code review
d081bb9 
Co-authored-by: Ryan <ryanford@poluglottos.com>
@walldiss
improve comments
5043a4c
@walldiss @distractedm1nd
Apply suggestions from code review
95f15f4 
Co-authored-by: Ryan <ryanford@poluglottos.com>
@walldiss @renaynay
Apply suggestions from code review
830dfbf 
Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>
@walldiss
- rename bufferedODSReader
a83528d 
 - add header length in error
@walldiss
- update comments
6abd657 
 - update ODSReader to use io.Reader
 - add reconstruction test
 - utilise provided container as much as possible before returning it
renaynay
renaynay reviewed Nov 28, 2022
View reviewed changes
share/eds/ods.go Outdated Show resolved Hide resolved
@walldiss walldiss dismissed stale reviews from vgonkivs and Wondertan via 95c3940 November 28, 2022 12:16
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.

lfg

@walldiss walldiss enabled auto-merge (squash) November 28, 2022 12:44
@walldiss walldiss merged commit c71c036 into celestiaorg:main Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MSevey MSevey MSevey left review comments

@Wondertan Wondertan Wondertan approved these changes

@renaynay renaynay renaynay approved these changes

@distractedm1nd distractedm1nd distractedm1nd left review comments

@vgonkivs vgonkivs vgonkivs left review comments

Assignees

@walldiss walldiss

Labels
area:shares Shares and samples kind:feat Attached to feature PRs
Projects
No open projects
Celestia Node
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat(share/eds): ODSReader
7 participants
@walldiss @MSevey @Wondertan @distractedm1nd @codecov-commenter @vgonkivs @renaynay