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

Execute clean indexes in finally #3772

Merged
merged 1 commit into from
Apr 7, 2023

Conversation

gaozhangmin
Copy link
Contributor

Motivation

Fixes #3771

Changes

Move cleanupExecutor to finally block to make sure it can be executed every checkpoint process.

@codecov-commenter
Copy link

Codecov Report

Merging #3772 (c65b06d) into master (b85ac48) will decrease coverage by 20.65%.
The diff coverage is 55.55%.

@@              Coverage Diff              @@
##             master    #3772       +/-   ##
=============================================
- Coverage     68.07%   47.43%   -20.65%     
+ Complexity     6648     4508     -2140     
=============================================
  Files           467      467               
  Lines         40829    40829               
  Branches       5234     5234               
=============================================
- Hits          27795    19366     -8429     
- Misses        10772    19467     +8695     
+ Partials       2262     1996      -266     
Flag Coverage Δ
bookie ?
client ?
remaining 29.41% <0.00%> (-0.05%) ⬇️
replication 41.42% <55.55%> (+0.03%) ⬆️
tls 21.09% <0.00%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ie/storage/ldb/SingleDirectoryDbLedgerStorage.java 23.51% <55.55%> (-44.08%) ⬇️
...java/org/apache/bookkeeper/proto/BookieClient.java 0.00% <0.00%> (-100.00%) ⬇️
...a/org/apache/bookkeeper/client/api/BookKeeper.java 0.00% <0.00%> (-100.00%) ⬇️
...org/apache/bookkeeper/bookie/ReadOnlyFileInfo.java 0.00% <0.00%> (-100.00%) ⬇️
...org/apache/bookkeeper/bookie/datainteg/Events.java 0.00% <0.00%> (-100.00%) ⬇️
...rg/apache/bookkeeper/client/api/CreateBuilder.java 0.00% <0.00%> (-100.00%) ⬇️
.../apache/bookkeeper/bookie/storage/EntryLogger.java 0.00% <0.00%> (-100.00%) ⬇️
.../bookkeeper/proto/checksum/DummyDigestManager.java 0.00% <0.00%> (-100.00%) ⬇️
...okkeeper/client/BookieAddressResolverDisabled.java 0.00% <0.00%> (-100.00%) ⬇️
...keeper/proto/ReadLastConfirmedAndEntryContext.java 0.00% <0.00%> (-100.00%) ⬇️
... and 226 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@hangc0276 hangc0276 left a comment

Choose a reason for hiding this comment

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

What's the difference between moving the code block into finally?

Copy link
Contributor

@StevenLuMT StevenLuMT left a comment

Choose a reason for hiding this comment

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

I think it doesn't solve the fundamental problem.

@gaozhangmin
Copy link
Contributor Author

gaozhangmin commented Feb 21, 2023

What's the difference between moving the code block into finally?

Moved to finally, The index clean up is going on when writeCache.isEmpty().
Which can address the issue i mentioned in #3771, When a bookie transits from readonly to write-read mode. lots of index cleanup would gather @hangc0276 @StevenLuMT

@gaozhangmin gaozhangmin requested review from hangc0276 and StevenLuMT and removed request for hangc0276 and StevenLuMT February 21, 2023 10:59
@gaozhangmin
Copy link
Contributor Author

@hangc0276 PTAL

@gaozhangmin gaozhangmin requested review from hangc0276 and removed request for StevenLuMT and hangc0276 March 30, 2023 07:50
Copy link
Contributor

@StevenLuMT StevenLuMT left a comment

Choose a reason for hiding this comment

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

lgtm

zymap pushed a commit that referenced this pull request Jun 19, 2023
Execute clean indexes in finally (#3772)

(cherry picked from commit 04e572b)
hangc0276 pushed a commit to hangc0276/bookkeeper that referenced this pull request Jun 26, 2023
Execute clean indexes in finally (apache#3772)

(cherry picked from commit 04e572b)
zymap pushed a commit that referenced this pull request Dec 6, 2023
Execute clean indexes in finally (#3772)

(cherry picked from commit 04e572b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

db-storage-cleanup would impact write request
5 participants