Impose a memory limit on the bookie journal #2710
Merged
Conversation
This file contains hidden or 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
codelipenghui
approved these changes
May 7, 2021
jiazhai
approved these changes
May 7, 2021
eolivelli
requested changes
May 7, 2021
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalMaxMemoryTest.java
Outdated
Show resolved
Hide resolved
Contributor
Author
As documented in the |
eolivelli
reviewed
May 7, 2021
bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/MemoryLimitController.java
Show resolved
Hide resolved
Contributor
Author
|
@eolivelli PTAL again |
Contributor
Author
|
@dlg99 I marked this on purpose for 4.14 |
hsaputra
reviewed
May 7, 2021
| # Set the max amount of memory that can be used by the journal. | ||
| # If empty, this will be set to use 5% of available direct memory | ||
| # Setting it to 0, it will disable the max memory control for the journal. | ||
| # journalMaxMemorySizeMb= |
Contributor
There was a problem hiding this comment.
So looks like this will be additional limit to t he max Journal Q size?
Contributor
Author
There was a problem hiding this comment.
Correct. that would still be in place
merlimat
pushed a commit
that referenced
this pull request
May 24, 2021
### Motivation After add label for prometheus metric by #2650, it will cause prometheus metric format check failed when no label specified for a statsLogger. The metric list as follow. ``` replication_bookkeeper_client_bookkeeper_client_bookie_watcher_NEW_ENSEMBLE_TIME{success="false",quantile="0.9999", } NaN ``` ### Modification 1. add label empty check for `PrometheusTextFormatUtil` 2. add label scope check test cover 3. add prometheus metric regex pattern check in test case Reviewers: lipenghui <penghui@apache.org>, Andrey Yegorov <None>, Matteo Merli <mmerli@apache.org>, Jia Zhai <zhaijia@apache.org>, Addison Higham <None>, Enrico Olivelli <eolivelli@gmail.com> This closes #2718 from hangc0276/chenhang/fix_bookeeper_metric_bug and squashes the following commits: 8590704 [hangc0276] format code a6942d4 [chenhang] fix prometheus metric provider bug and add test to cover label scope and metric format check bb8b1e0 [Andrey Yegorov] Include gradle files into the source artifact for releases, exclude site2/** 732b6cf [Andrey Yegorov] [maven-release-plugin] prepare for next development iteration b0d9f10 [Andrey Yegorov] [maven-release-plugin] prepare branch branch-4.14 8dc108b [Matteo Merli] y 73e22ca [Don Inghram] ISSUE2620: RocksDB log path configurable 034ef85 [Shoothzj] Fix logger member not correct; (#2605) b824a60 [hangc0276] fix always select the same region set bug for RegionAwareEnsemblePlacementPolicy (#2658) 683ad45 [Matteo Merli] Allow to attach labels to metrics (#2650) 8091096 [Matteo Merli] Allow to bypass journal for writes (#2401) 63867a9 [Matteo Merli] Impose a memory limit on the bookie journal (#2710) 87579b0 [Matteo Merli] Read entry error should print lastAddConfirmed in the log (#2707)
merlimat
pushed a commit
that referenced
this pull request
May 29, 2021
### Motivation After add label for prometheus metric by #2650, it will cause prometheus metric format check failed when no label specified for a statsLogger. The metric list as follow. ``` replication_bookkeeper_client_bookkeeper_client_bookie_watcher_NEW_ENSEMBLE_TIME{success="false",quantile="0.9999", } NaN ``` ### Modification 1. add label empty check for `PrometheusTextFormatUtil` 2. add label scope check test cover 3. add prometheus metric regex pattern check in test case Reviewers: lipenghui <penghui@apache.org>, Andrey Yegorov <None>, Matteo Merli <mmerli@apache.org>, Jia Zhai <zhaijia@apache.org>, Addison Higham <None>, Enrico Olivelli <eolivelli@gmail.com> This closes #2718 from hangc0276/chenhang/fix_bookeeper_metric_bug and squashes the following commits: 8590704 [hangc0276] format code a6942d4 [chenhang] fix prometheus metric provider bug and add test to cover label scope and metric format check bb8b1e0 [Andrey Yegorov] Include gradle files into the source artifact for releases, exclude site2/** 732b6cf [Andrey Yegorov] [maven-release-plugin] prepare for next development iteration b0d9f10 [Andrey Yegorov] [maven-release-plugin] prepare branch branch-4.14 8dc108b [Matteo Merli] y 73e22ca [Don Inghram] ISSUE2620: RocksDB log path configurable 034ef85 [Shoothzj] Fix logger member not correct; (#2605) b824a60 [hangc0276] fix always select the same region set bug for RegionAwareEnsemblePlacementPolicy (#2658) 683ad45 [Matteo Merli] Allow to attach labels to metrics (#2650) 8091096 [Matteo Merli] Allow to bypass journal for writes (#2401) 63867a9 [Matteo Merli] Impose a memory limit on the bookie journal (#2710) 87579b0 [Matteo Merli] Read entry error should print lastAddConfirmed in the log (#2707)
Sunny-Island
pushed a commit
to Sunny-Island/bookkeeper
that referenced
this pull request
Jun 2, 2021
### Motivation After add label for prometheus metric by apache#2650, it will cause prometheus metric format check failed when no label specified for a statsLogger. The metric list as follow. ``` replication_bookkeeper_client_bookkeeper_client_bookie_watcher_NEW_ENSEMBLE_TIME{success="false",quantile="0.9999", } NaN ``` ### Modification 1. add label empty check for `PrometheusTextFormatUtil` 2. add label scope check test cover 3. add prometheus metric regex pattern check in test case Reviewers: lipenghui <penghui@apache.org>, Andrey Yegorov <None>, Matteo Merli <mmerli@apache.org>, Jia Zhai <zhaijia@apache.org>, Addison Higham <None>, Enrico Olivelli <eolivelli@gmail.com> This closes apache#2718 from hangc0276/chenhang/fix_bookeeper_metric_bug and squashes the following commits: 8590704 [hangc0276] format code a6942d4 [chenhang] fix prometheus metric provider bug and add test to cover label scope and metric format check bb8b1e0 [Andrey Yegorov] Include gradle files into the source artifact for releases, exclude site2/** 732b6cf [Andrey Yegorov] [maven-release-plugin] prepare for next development iteration b0d9f10 [Andrey Yegorov] [maven-release-plugin] prepare branch branch-4.14 8dc108b [Matteo Merli] y 73e22ca [Don Inghram] ISSUE2620: RocksDB log path configurable 034ef85 [Shoothzj] Fix logger member not correct; (apache#2605) b824a60 [hangc0276] fix always select the same region set bug for RegionAwareEnsemblePlacementPolicy (apache#2658) 683ad45 [Matteo Merli] Allow to attach labels to metrics (apache#2650) 8091096 [Matteo Merli] Allow to bypass journal for writes (apache#2401) 63867a9 [Matteo Merli] Impose a memory limit on the bookie journal (apache#2710) 87579b0 [Matteo Merli] Read entry error should print lastAddConfirmed in the log (apache#2707)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
Motivation
Currently, the journal has a configurable max queue size, with default to 10,000 entries. This can be used as a proxy measure to control the max amount of memory that has to be retained from the entries that are in the journal queue.
This is not very flexible and there are few potential problems that arise:
Instead (or additionally) of number of entries, we should use the total amount of memory as the metric for enabling the backpressure.
Changes
Added
MemoryLimitControllerclass, already used in Pulsar (apache/pulsar#8965) and use it to block the thread adding to the journal queue when memory is exhausted.Defaulting to use 5% of max direct memory (eg: 100MB out of 2GB)