fix always select the same region set bug for RegionAwareEnsemblePlacementPolicy #2658
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
Contributor
Author
|
rerun failure checks |
eolivelli
requested changes
Mar 23, 2021
...er-server/src/main/java/org/apache/bookkeeper/client/RegionAwareEnsemblePlacementPolicy.java
Outdated
Show resolved
Hide resolved
| List<BookieId> ensemble2 = repp.newEnsemble(3, 3, 2, | ||
| null, new HashSet<>()).getResult(); | ||
| ensemble1.retainAll(ensemble2); | ||
| assert(ensemble1.size() >= 1); |
Contributor
There was a problem hiding this comment.
if we depend on a Random number, is this test going to be flaky ? probably not, but I would like to double check your thoughts
Contributor
Author
There was a problem hiding this comment.
if the lastRegionIndex initialized to specific value, all bookkeeper clients will start with the same region, which may lead to hotpot for regions. However those bookkeeper clients create ledger speed won't keep sync, starting will the same region won't make much difference. I changed the initialized value to 0.
Contributor
Author
|
rerun failure checks |
1 similar comment
Contributor
Author
|
rerun failure checks |
Contributor
Author
|
@eolivelli @merlimat @sijie Please take a look, thanks. |
merlimat
approved these changes
May 8, 2021
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
3 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
When use
RegionAwareEnsemblePlacementPolicyto select ensemble set, it always select the same region set for all ledger creation.The reason is that it traverse the availableRegions set in the same order for ensemble selection and select bookies in the same region set, which lead the load not balance between regions.
Changes
regionIndexfield to mark next select index for available region list, avoiding traverse from the same position for each ensemble selection.I will add the test latter.