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

Fix the infinite waiting for shutdown due to throttler limit #2942

Merged
merged 12 commits into from
Jul 28, 2022

Conversation

wenbingshen
Copy link
Member

@wenbingshen wenbingshen commented Dec 15, 2021

Descriptions of the changes in this PR:

Motivation

If the compactor is limited, the shutdown priority should be higher than waiting for RateLimiter.acquire.

Changes

According to @hangc0276 suggestion, when processing the shutdown logic of GarbageCollectorThread, we should check the status of the newScanner.process method. If the status is false, throw an IOException and stop compact immediately.

Master Issue: #2941

@wenbingshen
Copy link
Member Author

@eolivelli @merlimat @codelipenghui Can you take a look at this pr. Thanks.

@hangc0276
Copy link
Contributor

@wenbingshen Thanks for you enormous contribution.
The root cause of preventing GarbageCollectorThread shutdown is that the compacting flag only control one compact entry phase.

protected void compactEntryLog(EntryLogMetadata entryLogMeta) {
// Similar with Sync Thread
// try to mark compacting flag to make sure it would not be interrupted
// by shutdown during compaction. otherwise it will receive
// ClosedByInterruptException which may cause index file & entry logger
// closed and corrupted.
if (!compacting.compareAndSet(false, true)) {
// set compacting flag failed, means compacting is true now
// indicates that compaction is in progress for this EntryLogId.
return;
}
try {
// Do the actual compaction
compactor.compact(entryLogMeta);
} catch (Exception e) {
LOG.error("Failed to compact entry log {} due to unexpected error", entryLogMeta.getEntryLogId(), e);
} finally {
// Mark compaction done
compacting.set(false);
}
}

So, it will wait for one entry log file compact complete before GarbageCollectorThread shutdown. If we throttle the compaction in low throughput, it will take multi minutes to compaction 2GB entry log file.

IMO, we'd better introduce a new compact flag for AbstractLogCompactor and check the status on newScanner.process method, if the status is false, just throw an IOException, and stop the compact immediately. It can work for both EntryLogCompactor and TransactionalEntryLogCompactor.

@wenbingshen
Copy link
Member Author

@hangc0276 Gotcha! Thanks for your review. I will implement it soon. :)

@wenbingshen wenbingshen changed the title [BUGFIX] Fix the infinite waiting for shutdown due to throttler limit [WIP] Fix the infinite waiting for shutdown due to throttler limit Dec 23, 2021
@wenbingshen
Copy link
Member Author

rerun failure checks

2 similar comments
@wenbingshen
Copy link
Member Author

rerun failure checks

@wenbingshen
Copy link
Member Author

rerun failure checks

@wenbingshen
Copy link
Member Author

rerun failure checks

@wenbingshen
Copy link
Member Author

@hangc0276 PTAL, thanks. :)

@wenbingshen wenbingshen changed the title [WIP] Fix the infinite waiting for shutdown due to throttler limit Fix the infinite waiting for shutdown due to throttler limit Dec 29, 2021
Copy link
Contributor

@dlg99 dlg99 left a comment

Choose a reason for hiding this comment

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

LGTM

@wenbingshen
Copy link
Member Author

ping @eolivelli PTAL, Thanks.

@wenbingshen
Copy link
Member Author

@eolivelli PTAL Thanks

@wenbingshen
Copy link
Member Author

@pkumar-singh It's been a long time. Can you take a look at this pr? Thanks.

@wenbingshen
Copy link
Member Author

@eolivelli It's been over 3 months since this PR was created. Can you help take a look at this PR? Thanks very much!

@hangc0276
Copy link
Contributor

@wenbingshen Would you please rebase the master? thanks.

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.

Good job!

@hangc0276
Copy link
Contributor

https://github.com/apache/bookkeeper/runs/7519602108?check_suite_focus=true
The CI failed, Please take a look, thanks.

@hangc0276 hangc0276 merged commit 442e3bb into apache:master Jul 28, 2022
zymap pushed a commit that referenced this pull request Aug 1, 2022
Descriptions of the changes in this PR:

If the compactor is limited, the shutdown priority should be higher than waiting for RateLimiter.acquire.

According to @hangc0276 suggestion, when processing the shutdown logic of `GarbageCollectorThread`, we should check the status of the `newScanner.process` method. If the status is false, throw an `IOException` and stop compact immediately.

Master Issue: #2941

(cherry picked from commit 442e3bb)
@wenbingshen wenbingshen deleted the f_compacted_state branch April 27, 2023 16:25
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
…2942)

Descriptions of the changes in this PR:
### Motivation

If the compactor is limited, the shutdown priority should be higher than waiting for RateLimiter.acquire.

### Changes

According to @hangc0276 suggestion, when processing the shutdown logic of `GarbageCollectorThread`, we should check the status of the `newScanner.process` method. If the status is false, throw an `IOException` and stop compact immediately.


Master Issue: apache#2941
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.

None yet

5 participants