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

correctness fixes for the autobatch blockstore #7940

Merged
merged 1 commit into from
Jan 13, 2022

Conversation

Stebalien
Copy link
Member

  1. Simplify shutdown and make it idempotent by using a context.
  2. Make sure Flush actually fully flushes if the previous flush failed.
  3. Don't clear the flush batch if flushing fails.

@Stebalien Stebalien requested a review from a team as a code owner January 12, 2022 22:39
}

bs.doFlushLock.Lock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just to make sure that any earlier claimers of the lock finish before we return?

Copy link
Member Author

Choose a reason for hiding this comment

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

Concurrent Flush and Shutdown.

1. Simplify shutdown and make it idempotent by using a context.
2. Make sure `Flush` actually _fully_ flushes if the previous flush failed.
3. Don't clear the flush batch if flushing fails.
@Stebalien Stebalien force-pushed the steb/buffered-blockstore-correctness branch from c7246e1 to 2a862d4 Compare January 12, 2022 23:19
@Stebalien
Copy link
Member Author

(forgot to create the channel)

@codecov
Copy link

codecov bot commented Jan 13, 2022

Codecov Report

Merging #7940 (2a862d4) into master (b161f56) will increase coverage by 0.01%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7940      +/-   ##
==========================================
+ Coverage   39.32%   39.34%   +0.01%     
==========================================
  Files         658      658              
  Lines       71119    71122       +3     
==========================================
+ Hits        27970    27981      +11     
+ Misses      38332    38331       -1     
+ Partials     4817     4810       -7     
Impacted Files Coverage Δ
blockstore/autobatch.go 57.26% <75.00%> (-4.14%) ⬇️
chain/events/observer.go 71.64% <0.00%> (-6.72%) ⬇️
blockstore/buffered.go 34.44% <0.00%> (-2.23%) ⬇️
chain/store/store.go 64.16% <0.00%> (-1.34%) ⬇️
paychmgr/simple.go 79.32% <0.00%> (-0.93%) ⬇️
chain/sync_manager.go 66.77% <0.00%> (-0.63%) ⬇️
node/impl/full/chain.go 12.27% <0.00%> (-0.60%) ⬇️
node/impl/storminer.go 22.47% <0.00%> (-0.34%) ⬇️
miner/miner.go 58.36% <0.00%> (ø)
extern/sector-storage/sched_worker.go 78.48% <0.00%> (+0.29%) ⬆️
... and 10 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 b161f56...2a862d4. Read the comment docs.

@arajasek arajasek merged commit 3c57b98 into master Jan 13, 2022
@arajasek arajasek deleted the steb/buffered-blockstore-correctness branch January 13, 2022 00:17
@jennijuju
Copy link
Member

folllowing the PR template - a nice PR title should be created so our change log is pretty

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants