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(share): EDSStore scaffolding #1232

Merged
merged 25 commits into from
Nov 23, 2022

Conversation

distractedm1nd
Copy link
Collaborator

@distractedm1nd distractedm1nd commented Oct 14, 2022

Based on top of #1158, replaces #1051

Closes #1107
Closes #1108
Closes #1110
Closes #1111
Closes #1112

@distractedm1nd distractedm1nd added area:shares Shares and samples kind:feat Attached to feature PRs labels Oct 14, 2022
@distractedm1nd distractedm1nd self-assigned this Oct 14, 2022
@codecov-commenter
Copy link

codecov-commenter commented Oct 14, 2022

Codecov Report

Merging #1232 (8ce9631) into main (f8bef40) will decrease coverage by 0.30%.
The diff coverage is 55.00%.

@@            Coverage Diff             @@
##             main    #1232      +/-   ##
==========================================
- Coverage   55.42%   55.12%   -0.31%     
==========================================
  Files         178      180       +2     
  Lines       10746    10890     +144     
==========================================
+ Hits         5956     6003      +47     
- Misses       4211     4288      +77     
- Partials      579      599      +20     
Impacted Files Coverage Δ
nodebuilder/share/service.go 100.00% <ø> (ø)
share/eds/retriever.go 93.71% <ø> (ø)
share/eds/store.go 52.80% <52.80%> (ø)
libs/utils/address.go 59.09% <59.09%> (ø)
nodebuilder/gateway/config.go 68.42% <66.66%> (+5.92%) ⬆️
nodebuilder/rpc/config.go 66.66% <66.66%> (+6.66%) ⬆️
nodebuilder/core/config.go 57.14% <100.00%> (-5.36%) ⬇️
nodebuilder/core/opts.go 0.00% <0.00%> (-50.00%) ⬇️
nodebuilder/das/daser.go 75.00% <0.00%> (-25.00%) ⬇️
header/core/listener.go 52.83% <0.00%> (-5.67%) ⬇️
... and 3 more

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

share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Outdated 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.

  • can you doc all noops in blockstore and why they're noops (even if it's just a quick sentence and say check ADR for more info)

share/eds/store.go Show resolved Hide resolved
share/eds/store.go Outdated Show resolved Hide resolved
share/eds/store.go Outdated Show resolved Hide resolved
share/eds/store.go Outdated Show resolved Hide resolved
share/eds/store.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
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.

I deep review of EDStore only. I witsh Blockstore was a separate PR.

General comments for the EDSStore:

  • We should wrap errors in each method with meaningful messages via fmt.Errorf(%w)
    • We should start a habit of doing this more often. Our codebase lacks this, and usually, it is hard to grasp what happened from the error message and such wrappings should improve this.
  • Each result chan read must obey Context. Check one of the comments for more details on that
  • share.Root is actually a full blown DAHeader. The networking layer we build on top, won't have access to it, as we don't want to send it over the wire. Thus, we have to change it to the DataHash or DAHeader.Hash result. Initially, I wanted to ask to do the change in this PR, but then decided to do in a subsequent PR after we clarify this in the Storage ADR first. We also need to come up with cool alias name for the DataHash in share pkg 😂

share/eds/store.go Outdated Show resolved Hide resolved
share/eds/store.go Show resolved Hide resolved
share/eds/store.go Outdated Show resolved Hide resolved
share/eds/store.go Show resolved Hide resolved
share/eds/store.go Outdated Show resolved Hide resolved
share/eds/store.go Show resolved Hide resolved
share/eds/store.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
Copy link
Member

@walldiss walldiss left a comment

Choose a reason for hiding this comment

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

Looks good, awesome work!

I've added one small optimisation suggestion.

share/eds/blockstore.go Outdated Show resolved Hide resolved
@distractedm1nd
Copy link
Collaborator Author

Removed blockstore so it can go into a new PR and can be reviewed more thoroughly, this PR is too big rn and the main part of it can get merged

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.

There is one general comment still unaddressed from the last review:

  • We should wrap errors in each method with meaningful messages via fmt.Errorf(%w)
    • We should start a habit of doing this more often. Our codebase lacks this, and usually, it is hard to grasp what happened from the error message and such wrappings should improve this.

E.g. if the Remove method fails, wrappings will hint to us which actions specifically failed, as Remove consists of Destroying the shard, dropping the index, and Removing the OS file. Without wrappings it will unclear which of this actions errored out

share/eds/store_test.go Outdated Show resolved Hide resolved
share/eds/store.go Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
Wondertan
Wondertan previously approved these changes Nov 23, 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.

Awesome. Thanks man

share/eds/store.go Show resolved Hide resolved
share/eds/store.go Outdated Show resolved Hide resolved
Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>
Wondertan
Wondertan previously approved these changes Nov 23, 2022
share/eds/store.go Outdated Show resolved Hide resolved
share/eds/store.go Outdated Show resolved Hide resolved
Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>
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.

Looks good, i've already reviewed this and i guess blockstore has been removed so no real comments other than re topIdx

share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/store.go Show resolved Hide resolved
@distractedm1nd distractedm1nd merged commit e7ea1f1 into celestiaorg:main Nov 23, 2022
@distractedm1nd distractedm1nd linked an issue Nov 24, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:shares Shares and samples kind:feat Attached to feature PRs
Projects
No open projects
Status: Done
6 participants