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(eds/store): remove corrupted blocks from store #2625

Merged
merged 10 commits into from
Sep 5, 2023

Conversation

distractedm1nd
Copy link
Collaborator

@distractedm1nd distractedm1nd commented Aug 28, 2023

Related to #2335

If an OpShardFail is found or corruption is detected from GetSharesByNamespace, the shard is removed

@codecov-commenter
Copy link

codecov-commenter commented Aug 28, 2023

Codecov Report

Merging #2625 (6ebde81) into main (a3220f4) will decrease coverage by 0.11%.
Report is 2 commits behind head on main.
The diff coverage is 37.50%.

@@            Coverage Diff             @@
##             main    #2625      +/-   ##
==========================================
- Coverage   51.15%   51.05%   -0.11%     
==========================================
  Files         158      158              
  Lines       10558    10638      +80     
==========================================
+ Hits         5401     5431      +30     
- Misses       4681     4732      +51     
+ Partials      476      475       -1     
Files Changed Coverage Δ
blob/blob.go 62.68% <ø> (-6.21%) ⬇️
nodebuilder/share/constructors.go 8.92% <0.00%> (-1.28%) ⬇️
nodebuilder/share/module.go 0.00% <0.00%> (ø)
share/getters/store.go 72.41% <0.00%> (-10.49%) ⬇️
share/share.go 0.00% <0.00%> (ø)
share/eds/metrics.go 19.41% <28.57%> (+0.66%) ⬆️
share/p2p/peers/manager.go 72.18% <43.75%> (-1.84%) ⬇️
share/eds/store.go 63.75% <93.33%> (+3.75%) ⬆️
share/eds/accessor_cache.go 38.94% <100.00%> (+4.11%) ⬆️
share/p2p/peers/options.go 58.97% <100.00%> (+7.45%) ⬆️

... and 4 files with indirect coverage changes

@distractedm1nd distractedm1nd marked this pull request as ready for review August 28, 2023 18:14
Copy link
Contributor

@ramin ramin left a comment

Choose a reason for hiding this comment

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

made some minor comments from perspective of a new set of eyes on the project. I think logging shard failures and removals would be super useful for both a node runner, but also as it feels like this doesn't solve the issue, but is a step towards more work in this area.

share/eds/store_test.go Outdated Show resolved Hide resolved
share/p2p/peers/manager.go Outdated Show resolved Hide resolved
share/eds/store.go Show resolved Hide resolved
ramin
ramin previously approved these changes Aug 30, 2023
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, just some code structuring suggestions and one small issue

share/p2p/peers/manager.go Outdated Show resolved Hide resolved
share/p2p/peers/manager.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
nodebuilder/share/module.go Show resolved Hide resolved
share/getters/store.go Outdated Show resolved Hide resolved
share/p2p/peers/options.go Outdated Show resolved Hide resolved
share/p2p/peers/manager.go Outdated Show resolved Hide resolved
renaynay
renaynay previously approved these changes Aug 31, 2023
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.

Lint and pls check swamp tests locally

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.

Small one left

share/p2p/peers/manager.go Show resolved Hide resolved
@distractedm1nd distractedm1nd enabled auto-merge (squash) September 5, 2023 09:35
@distractedm1nd distractedm1nd merged commit 456d169 into main Sep 5, 2023
14 of 16 checks passed
@distractedm1nd distractedm1nd deleted the remove-corrupted-blocks branch September 5, 2023 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:storage kind:feat Attached to feature PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants