feat(blocksync): share.WriteEDS
#1139
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
distractedm1nd
added
area:shares
distractedm1nd
force-pushed
the
writeeds
branch
3 times, most recently
from
0a91836
to
03df83b
Compare
distractedm1nd
changed the title
share.WriteEDS share.WriteEDS
distractedm1nd
requested review from
renaynay,
Wondertan and
vgonkivs
as code owners
Codecov Report
@@ Coverage Diff @@
## main #1139 +/- ##
==========================================
+ Coverage 55.86% 55.92% +0.05%
==========================================
Files 160 161 +1
Lines 9725 9869 +144
==========================================
+ Hits 5433 5519 +86
- Misses 3756 3796 +40
- Partials 536 554 +18
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
distractedm1nd
commented
Sep 22, 2022
vgonkivs
reviewed
Sep 30, 2022
vgonkivs
reviewed
Sep 30, 2022
vgonkivs
reviewed
Sep 30, 2022
vgonkivs
reviewed
Sep 30, 2022
Wondertan
reviewed
Oct 3, 2022
There was a problem hiding this comment.
Looks correct(besides that byte removal) There is a bunch of optimization that we will have to do eventually, but it's fine for now. Tmrw I will take a deep dive into that bytes mystery and how it works without it. Also, fmt needs to be done in here.
We need to test if it is indexable by DAGStore. Doesn't have to import the whole DAGStore for now and the test can only use car stuff.
Wondertan
added a commit
that referenced
this pull request
Oct 4, 2022
The code has a special case for the data with type byte and without. However, it truncates the type byte like it always there. The #1139 is the first user of data without type byte which uncovered the bug. The type byte causes issues for us again.
Wondertan
added a commit
that referenced
this pull request
Oct 4, 2022
The code has a special case for the share data with type byte and without. However, it truncates the type byte like it is always there. The #1139 is the first user of share data without a type byte, which uncovered the bug. The type byte causes issues for us again.
distractedm1nd
force-pushed
the
writeeds
branch
2 times, most recently
from
0c743f9
to
76241f1
Compare
Wondertan
added a commit
that referenced
this pull request
Oct 5, 2022
The code has a special case for the share data with type byte and without. However, it truncates the type byte like it is always there. The #1139 is the first user of share data without a type byte, which uncovered the bug. The type byte causes issues for us again. refactor(ipld/plugin): extract namespaceHasher into a separate file with some additional cleanups In preparation for the upcoming test for the namespaceHasher, we extract it into a separate file. Also, some additional cosmetics were done reducing the API surfuce of the pkg, hiding things that are not supposed to be used outside. test(ipld/plugin): write dummy unit test for the namespaceHasher
Wondertan
added a commit
that referenced
this pull request
Oct 5, 2022
The code has a special case for the share data with type byte and without. However, it truncates the type byte like it is always there. The #1139 is the first user of share data without a type byte, which uncovered the bug. The type byte causes issues for us again. refactor(ipld/plugin): extract namespaceHasher into a separate file with some additional cleanups In preparation for the upcoming test for the namespaceHasher, we extract it into a separate file. Also, some additional cosmetics were done reducing the API surfuce of the pkg, hiding things that are not supposed to be used outside. test(ipld/plugin): write dummy unit test for the namespaceHasher docs(ipld/plugin): godocs for the namespaceHasher perf(ipld/plugin): use sha256-simd to gain 40% speedup for hash computations
Wondertan
added a commit
that referenced
this pull request
Oct 5, 2022
The code has a special case for the share data with type byte and without. However, it truncates the type byte like it is always there. The #1139 is the first user of share data without a type byte, which uncovered the bug. The type byte causes issues for us again. refactor(ipld/plugin): extract namespaceHasher into a separate file with some additional cleanups In preparation for the upcoming test for the namespaceHasher, we extract it into a separate file. Also, some additional cosmetics were done reducing the API surfuce of the pkg, hiding things that are not supposed to be used outside. test(ipld/plugin): write dummy unit test for the namespaceHasher docs(ipld/plugin): godocs for the namespaceHasher perf(ipld/plugin): use sha256-simd to gain 40% speedup for hash computations
Wondertan
reviewed
Oct 5, 2022
distractedm1nd
force-pushed
the
writeeds
branch
from
76241f1
to
0237fcb
Compare
Wondertan
previously approved these changes
Oct 10, 2022
There was a problem hiding this comment.
LGTM. Really have no comments here. Great work.
- We need to hold it off before consolidation is merged and then rebase it on top
- I would still suggest testing indexing explicitly here, but definitely not blocking
- There is a bunch of optimizations(and related refactorings) we can do here, but they are also not blocking and can be done much further
distractedm1nd
force-pushed
the
writeeds
branch
from
0237fcb
to
04afc9a
Compare
Wondertan
previously approved these changes
Oct 14, 2022
distractedm1nd
force-pushed
the
writeeds
branch
from
04afc9a
to
4b6e47c
Compare
… of writingSession
Co-authored-by: Viacheslav <viacheslavgonkivskyi@gmail.com>
distractedm1nd
force-pushed
the
writeeds
branch
from
25977d0
to
8b8ccf3
Compare
renaynay
reviewed
Oct 17, 2022
Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>
renaynay
approved these changes
Oct 18, 2022
walldiss
reviewed
Oct 18, 2022
Wondertan
approved these changes
Oct 19, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #1105
Still a few todos to be solved and comments to be added, but its ready for the first round of reviews now
Will change the base branch of the PR to the feature branch when it is set up (I will wait until there is preliminary work on it)