You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2022/12/27 09:31:26 UTC
[GitHub] [bookkeeper] horizonzy opened a new pull request, #3721: New ensemble choose different rack first.
horizonzy opened a new pull request, #3721:
URL: https://github.com/apache/bookkeeper/pull/3721
Descriptions of the changes in this PR:
Fixes #3720
### Motivation
When new ensemble, choose the different rack node first, if there are no more different rack nodes, step down the origin logic to pick nodes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] StevenLuMT commented on a diff in pull request #3721: New ensemble choose different rack first.
Posted by GitBox <gi...@apache.org>.
StevenLuMT commented on code in PR #3721:
URL: https://github.com/apache/bookkeeper/pull/3721#discussion_r1059265879
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java:
##########
@@ -414,21 +414,52 @@ protected PlacementResult<List<BookieId>> newEnsembleInternal(
}
return PlacementResult.of(addrs, PlacementPolicyAdherence.FAIL);
}
-
- for (int i = 0; i < ensembleSize; i++) {
- String curRack;
- if (null == prevNode) {
- if ((null == localNode) || defaultRack.equals(localNode.getNetworkLocation())) {
- curRack = NodeBase.ROOT;
+ try {
+ //Choose different rack nodes.
+ String curRack = null;
+ for (int i = 0; i < ensembleSize; i++) {
+ if (null == prevNode) {
+ if ((null == localNode) || defaultRack.equals(localNode.getNetworkLocation())) {
+ curRack = NodeBase.ROOT;
+ } else {
+ curRack = localNode.getNetworkLocation();
+ }
} else {
- curRack = localNode.getNetworkLocation();
+ if (!curRack.startsWith("~")) {
Review Comment:
"~" is the magic string
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on pull request #3721: New ensemble choose different rack first.
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on PR #3721:
URL: https://github.com/apache/bookkeeper/pull/3721#issuecomment-1383415127
ping @eolivelli @dlg99, please help take a look at this pr, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on a diff in pull request #3721: New ensemble choose different rack first.
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on code in PR #3721:
URL: https://github.com/apache/bookkeeper/pull/3721#discussion_r1059217690
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java:
##########
@@ -414,21 +414,52 @@ protected PlacementResult<List<BookieId>> newEnsembleInternal(
}
return PlacementResult.of(addrs, PlacementPolicyAdherence.FAIL);
}
-
- for (int i = 0; i < ensembleSize; i++) {
- String curRack;
- if (null == prevNode) {
- if ((null == localNode) || defaultRack.equals(localNode.getNetworkLocation())) {
- curRack = NodeBase.ROOT;
+ try {
+ //Choose different rack nodes.
+ String curRack = null;
+ for (int i = 0; i < ensembleSize; i++) {
+ if (null == prevNode) {
+ if ((null == localNode) || defaultRack.equals(localNode.getNetworkLocation())) {
+ curRack = NodeBase.ROOT;
+ } else {
+ curRack = localNode.getNetworkLocation();
+ }
} else {
- curRack = localNode.getNetworkLocation();
+ if (!curRack.startsWith("~")) {
+ curRack = "~" + prevNode.getNetworkLocation();
+ } else {
+ curRack = curRack + NetworkTopologyImpl.NODE_SEPARATOR + prevNode.getNetworkLocation();
+ }
}
- } else {
- curRack = "~" + prevNode.getNetworkLocation();
+ prevNode = selectRandomFromRack(curRack, excludeNodes, ensemble, ensemble);
Review Comment:
Can we set `fallbackRandom` flag in `selectRandomFromRack` to support random selection instead of random select in BKNotEnoughBookiesException
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] wenbingshen commented on pull request #3721: New ensemble choose different rack first.
Posted by "wenbingshen (via GitHub)" <gi...@apache.org>.
wenbingshen commented on PR #3721:
URL: https://github.com/apache/bookkeeper/pull/3721#issuecomment-1411771767
> After this pr, the traffic carried by rackA is still twice that of rackB.
I got. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on a diff in pull request #3721: New ensemble choose different rack first.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #3721:
URL: https://github.com/apache/bookkeeper/pull/3721#discussion_r1063938807
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java:
##########
@@ -414,21 +414,33 @@ protected PlacementResult<List<BookieId>> newEnsembleInternal(
}
return PlacementResult.of(addrs, PlacementPolicyAdherence.FAIL);
}
-
+ //Choose different rack nodes.
+ String curRack = null;
for (int i = 0; i < ensembleSize; i++) {
- String curRack;
if (null == prevNode) {
if ((null == localNode) || defaultRack.equals(localNode.getNetworkLocation())) {
curRack = NodeBase.ROOT;
} else {
curRack = localNode.getNetworkLocation();
}
} else {
- curRack = "~" + prevNode.getNetworkLocation();
+ if (!curRack.startsWith("~")) {
+ curRack = "~" + prevNode.getNetworkLocation();
+ } else {
+ curRack = curRack + NetworkTopologyImpl.NODE_SEPARATOR + prevNode.getNetworkLocation();
+ }
+ }
+ try {
+ prevNode = selectRandomFromRack(curRack, excludeNodes, ensemble, ensemble);
Review Comment:
Use `selectRandomFromRack` instead of `selectFromNetworkLocation`.
There are two reasons:
1. `selectFromNetworkLocation` params `fallbackToRandom` may step down to pick a node randomly.
2. If the user config `setNetworkTopologyStabilizePeriodSeconds`, RackawareEnsemblePlacementPolicy#selectFromNetworkLocation may step down to pick node from slave.
We should avoid it, just pick different node without steping down.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] wenbingshen commented on pull request #3721: New ensemble choose different rack first.
Posted by "wenbingshen (via GitHub)" <gi...@apache.org>.
wenbingshen commented on PR #3721:
URL: https://github.com/apache/bookkeeper/pull/3721#issuecomment-1408594064
@horizonzy Nice catch!
I have a question, if the user has a 30-node bookie cluster, 20 of which are rackA, and the other 10 are rackB, and the user has not enabled minNumRacksPerWriteQuorum and enforceMinNumRacksPerWriteQuorum;
Now the user upgrades the pulsar cluster, the modification brought by this PR will be enabled,
Will it happen that the data between the two racks is basically evenly distributed, resulting in double the traffic and storage pressure of rackB with only 10 nodes?
In the random selection process of the old version, in theory, the traffic carried by rackA should be twice that of rackB.
Do we need to pay attention to this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on a diff in pull request #3721: New ensemble choose different rack first.
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on code in PR #3721:
URL: https://github.com/apache/bookkeeper/pull/3721#discussion_r1059223675
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestRackawareEnsemblePlacementPolicy.java:
##########
@@ -1519,6 +1520,53 @@ public void testNewEnsembleWithMultipleRacks() throws Exception {
}
}
+ @Test
+ public void testNewEnsembleWithPickDifferentRack() throws Exception {
Review Comment:
Please also add unit test for region aware placement policy
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on pull request #3721: New ensemble choose different rack first.
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on PR #3721:
URL: https://github.com/apache/bookkeeper/pull/3721#issuecomment-1410127159
> @horizonzy Nice catch!
>
> I have a question, if the user has a 30-node bookie cluster, 20 of which are rackA, and the other 10 are rackB, and the user has not enabled minNumRacksPerWriteQuorum and enforceMinNumRacksPerWriteQuorum;
>
> Now the user upgrades the pulsar cluster, the modification brought by this PR will be enabled, Will it happen that the data between the two racks is basically evenly distributed, resulting in double the traffic and storage pressure of rackB with only 10 nodes?
>
> In the random selection process of the old version, in theory, the traffic carried by rackA should be twice that of rackB.
>
> Do we need to pay attention to this?
After this pr, the traffic carried by rackA is still twice that of rackB.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 merged pull request #3721: New ensemble choose different rack first.
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 merged PR #3721:
URL: https://github.com/apache/bookkeeper/pull/3721
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org