You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by "hangc0276 (via GitHub)" <gi...@apache.org> on 2023/08/22 16:00:30 UTC
[GitHub] [bookkeeper] hangc0276 opened a new pull request, #4057: Rackaware placement policy support local node awareness by hostname
hangc0276 opened a new pull request, #4057:
URL: https://github.com/apache/bookkeeper/pull/4057
### Motivation
Rack-aware placement policies enable preference for bookies that reside in the same rack as the bookie client.
- Initiate local node by resolving the rack information
https://github.com/apache/bookkeeper/blob/5b5c05331757e7356579076970e61f119f5d34ae/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java#L207-L215
- When generating new ensembles for a ledger, the selecting algorithm will set the localNode's rack to `curRack` and select one bookie from `curRack` first
https://github.com/apache/bookkeeper/blob/5b5c05331757e7356579076970e61f119f5d34ae/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java#L420-L426
However, when resolving the local node's rack information, we use IP to resolve the rack name, which is unfriendly with k8s deployment.
https://github.com/apache/bookkeeper/blob/5b5c05331757e7356579076970e61f119f5d34ae/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java#L209
In k8s deployment, we usually use the hostname as bookieId and Pulsar broker name instead of IP, because the IP will be changed when the pods migrated to other nodes.
### Modification
In order not to bring break change to the current behavior, I introduced a flag `useHostnameResolveLocalNodePlacementPolicy` in the BookKeeper client configuration to control whether to use the hostname to resolve the bookie client's local node rack information. The flag is `false` by default, which is the same behavior as the current logic.
Due to this PR doesn't introduce any break changes, I think we can cherry-pick it back to the patch releases (branch-4.14, branch-4.15 and branch-4.16)
--
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 #4057: Rackaware placement policy support local node awareness by hostname
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on PR #4057:
URL: https://github.com/apache/bookkeeper/pull/4057#issuecomment-1688492373
@horizonzy Please help take a look, 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 merged pull request #4057: Rackaware placement policy support local node awareness by hostname
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 merged PR #4057:
URL: https://github.com/apache/bookkeeper/pull/4057
--
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 #4057: Rackaware placement policy support local node awareness by hostname
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on code in PR #4057:
URL: https://github.com/apache/bookkeeper/pull/4057#discussion_r1306866548
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestRackawareEnsemblePlacementPolicy.java:
##########
@@ -1689,6 +1689,84 @@ public void testNewEnsembleWithPickDifferentRack() throws Exception {
}
}
+ @Test
+ public void testNewEnsemblePickLocalRackBookiesByHostname() throws Exception {
Review Comment:
This test cannot cover new features, whether there are new features or not, the test will pass. I think we should check if the host and rack information of the localNode align with the expectations.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicy.java:
##########
@@ -57,21 +57,23 @@ protected RackawareEnsemblePlacementPolicy initialize(DNSToSwitchMapping dnsReso
int minNumRacksPerWriteQuorum,
boolean enforceMinNumRacksPerWriteQuorum,
boolean ignoreLocalNodeInPlacementPolicy,
+ boolean useHostnameResolveLocalNodePlacementPolicy,
Review Comment:
For compatibility, we can add a new method with the new param `useHostnameResolveLocalNodePlacementPolicy`. The origin method was just direct to the new method with the default value `false`.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java:
##########
@@ -1010,6 +1014,22 @@ public boolean getIgnoreLocalNodeInPlacementPolicy() {
return getBoolean(IGNORE_LOCAL_NODE_IN_PLACEMENT_POLICY, false);
}
+ /**
+ * Set the flag to use hostname to resolve local node placement policy.
+ * @param useHostnameResolveLocalNodePlacementPolicy
+ */
+ public void setUseHostnameResolveLocalNodePlacementPolicy(boolean useHostnameResolveLocalNodePlacementPolicy) {
+ setProperty(USE_HOSTNAME_RESOLVE_LOCAL_NODE_PLACEMENT_POLICY, useHostnameResolveLocalNodePlacementPolicy);
+ }
+
+ /**
+ * Get whether to use hostname to resolve local node placement policy.
+ * @return
+ */
+ public boolean getUseHostnameResolveLocalNodePlacementPolicy() {
+ return getBoolean(USE_HOSTNAME_RESOLVE_LOCAL_NODE_PLACEMENT_POLICY, false);
+ }
Review Comment:
+1
--
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] zymap commented on a diff in pull request #4057: Rackaware placement policy support local node awareness by hostname
Posted by "zymap (via GitHub)" <gi...@apache.org>.
zymap commented on code in PR #4057:
URL: https://github.com/apache/bookkeeper/pull/4057#discussion_r1305012424
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java:
##########
@@ -160,6 +161,7 @@ protected RackawareEnsemblePlacementPolicyImpl initialize(DNSToSwitchMapping dns
int minNumRacksPerWriteQuorum,
boolean enforceMinNumRacksPerWriteQuorum,
boolean ignoreLocalNodeInPlacementPolicy,
+ boolean useHostnameResolveLocalNodePlacementPolicy,
Review Comment:
Same with above.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicy.java:
##########
@@ -57,21 +57,23 @@ protected RackawareEnsemblePlacementPolicy initialize(DNSToSwitchMapping dnsReso
int minNumRacksPerWriteQuorum,
boolean enforceMinNumRacksPerWriteQuorum,
boolean ignoreLocalNodeInPlacementPolicy,
+ boolean useHostnameResolveLocalNodePlacementPolicy,
Review Comment:
It is a `protected` method, changing the method signature will introduce a break if we want to back-support the old versions. We could mark it to deprecate this method and overload it. Then remove it in the future versions.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java:
##########
@@ -1010,6 +1014,22 @@ public boolean getIgnoreLocalNodeInPlacementPolicy() {
return getBoolean(IGNORE_LOCAL_NODE_IN_PLACEMENT_POLICY, false);
}
+ /**
+ * Set the flag to use hostname to resolve local node placement policy.
+ * @param useHostnameResolveLocalNodePlacementPolicy
+ */
+ public void setUseHostnameResolveLocalNodePlacementPolicy(boolean useHostnameResolveLocalNodePlacementPolicy) {
+ setProperty(USE_HOSTNAME_RESOLVE_LOCAL_NODE_PLACEMENT_POLICY, useHostnameResolveLocalNodePlacementPolicy);
+ }
+
+ /**
+ * Get whether to use hostname to resolve local node placement policy.
+ * @return
+ */
+ public boolean getUseHostnameResolveLocalNodePlacementPolicy() {
+ return getBoolean(USE_HOSTNAME_RESOLVE_LOCAL_NODE_PLACEMENT_POLICY, false);
+ }
Review Comment:
Since it is a client configuration, why not move it to the ClientConfiguration?
--
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] zymap commented on a diff in pull request #4057: Rackaware placement policy support local node awareness by hostname
Posted by "zymap (via GitHub)" <gi...@apache.org>.
zymap commented on code in PR #4057:
URL: https://github.com/apache/bookkeeper/pull/4057#discussion_r1305012194
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicy.java:
##########
@@ -57,21 +57,23 @@ protected RackawareEnsemblePlacementPolicy initialize(DNSToSwitchMapping dnsReso
int minNumRacksPerWriteQuorum,
boolean enforceMinNumRacksPerWriteQuorum,
boolean ignoreLocalNodeInPlacementPolicy,
+ boolean useHostnameResolveLocalNodePlacementPolicy,
Review Comment:
It is a `protected` method, changing the method signature will introduce a break. If we want to back-support the old versions, we could mark it to deprecate this method and overload it. Then remove it in the future versions.
--
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