aloyszhang commented on a change in pull request #14248:
URL: https://github.com/apache/pulsar/pull/14248#discussion_r805225205
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMapping.java
##########
@@ -201,7 +216,16 @@ private void handleUpdates(Notification n) {
bookieAddressList.add(BookieId.parse(addr));
}
}
- rackawarePolicy.onBookieRackChange(bookieAddressList);
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Bookies with rack update from {} to
{}", bookieAddressListLastTime,
+ bookieAddressList);
+ }
+ if (bookieAddressListLastTime.size() >
bookieAddressList.size()) {
+
rackawarePolicy.onBookieRackChange(bookieAddressListLastTime);
Review comment:
This method will be called if there are any changes of content under
znode`/bookies`. So, if the contents are exactly the same, this method will not
be called.
The change of bookies witch rack has three types: add (add a new bookie with
rack), delete (delete an existing bookie with rack), update (update rack for a
bookie).
Before this pull request, `handleUpdates` can only deal with add and update,
this is because if delete one bookie with rack, the `bookieAddressList` to
notify `rackawarePolicy` does not contain the deleted bookie leading to the
`NetworkTopology` can't delete the racks of this deleted bookie. And then, this
deleted bookie can be still picked into the ensemble.
This pull request resolves this problem, if we delete one bookie with rack,
when notifying `rackawarePolicy`, the `bookieAddressList` contains the deleted
bookie and all other bookies, so `NetworkTopology` can hear the change(delete)
and remove bookie from rack. After this, this deleted bookie can't be selected
for a new ensemble
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMapping.java
##########
@@ -201,7 +216,16 @@ private void handleUpdates(Notification n) {
bookieAddressList.add(BookieId.parse(addr));
}
}
- rackawarePolicy.onBookieRackChange(bookieAddressList);
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Bookies with rack update from {} to
{}", bookieAddressListLastTime,
+ bookieAddressList);
+ }
+ if (bookieAddressListLastTime.size() >
bookieAddressList.size()) {
+
rackawarePolicy.onBookieRackChange(bookieAddressListLastTime);
Review comment:
The`handleUpdates` method will be called if there are any changes of
content under znode`/bookies`. So, if the contents are exactly the same, this
method will not be called.
The change of bookies witch rack has three types: add (add a new bookie with
rack), delete (delete an existing bookie with rack), update (update rack for a
bookie).
Before this pull request, `handleUpdates` can only deal with add and update,
this is because if delete one bookie with rack, the `bookieAddressList` to
notify `rackawarePolicy` does not contain the deleted bookie leading to the
`NetworkTopology` can't delete the racks of this deleted bookie. And then, this
deleted bookie can be still picked into the ensemble.
This pull request resolves this problem, if we delete one bookie with rack,
when notifying `rackawarePolicy`, the `bookieAddressList` contains the deleted
bookie and all other bookies, so `NetworkTopology` can hear the change(delete)
and remove bookie from rack. After this, this deleted bookie can't be selected
for a new ensemble
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMapping.java
##########
@@ -77,6 +78,20 @@ public void setConf(Configuration conf) {
bookieMappingCache =
store.getMetadataCache(BookiesRackConfiguration.class);
bookieMappingCache.get(BOOKIE_INFO_ROOT_PATH).join();
+ try {
+ for (Map<String, BookieInfo> bookieMapping :
bookieMappingCache.get(BOOKIE_INFO_ROOT_PATH).get()
+ .map(Map::values).orElse(Collections.emptyList())) {
+ for (String address : bookieMapping.keySet()) {
+ bookieAddressListLastTime.add(BookieId.parse(address));
+ }
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("BookieRackAffinityMapping init,
bookieAddressListLastTime {}",
+ bookieAddressListLastTime);
+ }
+ }
+ } catch (InterruptedException | ExecutionException e) {
+ LOG.warn("Failed to init BookieId list", e);
Review comment:
Sure
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMapping.java
##########
@@ -77,6 +78,20 @@ public void setConf(Configuration conf) {
bookieMappingCache =
store.getMetadataCache(BookiesRackConfiguration.class);
bookieMappingCache.get(BOOKIE_INFO_ROOT_PATH).join();
+ try {
+ for (Map<String, BookieInfo> bookieMapping :
bookieMappingCache.get(BOOKIE_INFO_ROOT_PATH).get()
+ .map(Map::values).orElse(Collections.emptyList())) {
+ for (String address : bookieMapping.keySet()) {
+ bookieAddressListLastTime.add(BookieId.parse(address));
+ }
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("BookieRackAffinityMapping init,
bookieAddressListLastTime {}",
+ bookieAddressListLastTime);
+ }
+ }
+ } catch (InterruptedException | ExecutionException e) {
+ LOG.warn("Failed to init BookieId list", e);
Review comment:
Sure, thx
##########
File path:
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/RackAwareTest.java
##########
@@ -201,5 +201,121 @@ public void
testPlacementMinRackNumsPerWriteQuorum(boolean forceMinRackNums) thr
}
}
+ public void testRackUpdate() throws Exception {
+ // 1. reset configurations for rack-aware
+ cleanup();
+ config = new ServiceConfiguration();
+ config.setBookkeeperClientMinNumRacksPerWriteQuorum(2);
+ config.setBookkeeperClientEnforceMinNumRacksPerWriteQuorum(true);
+ setup();
+
+ // 2. test create ledger(ensemble size = 2) with only one rack
+ // rack-0 bookie-1
+ // rack-0 bookie-2
+ // rack-0 bookie-3
+
+ final String group = "default";
+ for (int i = 0; i < NUM_BOOKIES / 2; i++) {
+ String bookie = bookies.get(i).getLocalAddress().toString();
+ BookieInfo bi = BookieInfo.builder()
+ .rack("rack-0")
+ .hostname("bookie-" + (i + 1))
+ .build();
+ log.info("setting rack for bookie at {} -- {}", bookie, bi);
+ admin.bookies().updateBookieRackInfo(bookie, group, bi);
Review comment:
Yes, this call is blocking, but I think this blocking is to wait for the
request send to the broker successfully.
The `org.apache.pulsar.broker.admin.v2.Bookies#updateBookieRackInfo` is a
method with `void` result, so the Admin CLI doesn't wait for all the
operations finished on the broker side.
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMapping.java
##########
@@ -201,7 +216,16 @@ private void handleUpdates(Notification n) {
bookieAddressList.add(BookieId.parse(addr));
}
}
- rackawarePolicy.onBookieRackChange(bookieAddressList);
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Bookies with rack update from {} to
{}", bookieAddressListLastTime,
+ bookieAddressList);
+ }
+ if (bookieAddressListLastTime.size() >
bookieAddressList.size()) {
+
rackawarePolicy.onBookieRackChange(bookieAddressListLastTime);
Review comment:
The`handleUpdates` method will be called if there are any changes of
content under znode`/bookies`. So, if the contents are exactly the same, this
method will not be called.
The change of bookies with rack has three types: add (add a new bookie with
rack), delete (delete an existing bookie with rack), update (update rack for a
bookie).
Before this pull request, `handleUpdates` can only deal with add and update,
this is because if delete one bookie with rack, the `bookieAddressList` to
notify `rackawarePolicy` does not contain the deleted bookie leading to the
`NetworkTopology` can't delete the racks of this deleted bookie. And then, this
deleted bookie can be still picked into the ensemble.
This pull request resolves this problem, if we delete one bookie with rack,
when notifying `rackawarePolicy`, the `bookieAddressList` contains the deleted
bookie and all other bookies, so `NetworkTopology` can hear the change(delete)
and remove bookie from rack. After this, this deleted bookie can't be selected
for a new ensemble
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMapping.java
##########
@@ -201,7 +216,16 @@ private void handleUpdates(Notification n) {
bookieAddressList.add(BookieId.parse(addr));
}
}
- rackawarePolicy.onBookieRackChange(bookieAddressList);
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Bookies with rack update from {} to
{}", bookieAddressListLastTime,
+ bookieAddressList);
+ }
+ if (bookieAddressListLastTime.size() >
bookieAddressList.size()) {
+
rackawarePolicy.onBookieRackChange(bookieAddressListLastTime);
Review comment:
The`handleUpdates` method will be called if there are any changes of
content under znode`/bookies`. So, if the contents are exactly the same, this
method will not be called.
The change of bookies with rack has three types: add (add a rack for a
bookie ), delete (delete an existing rack for a bookie), update (update rack
for a bookie).
Before this pull request, `handleUpdates` can only deal with add and update,
this is because if delete one bookie with rack, the `bookieAddressList` to
notify `rackawarePolicy` does not contain the deleted bookie leading to the
`NetworkTopology` can't delete the racks of this deleted bookie. And then, this
deleted bookie can be still picked into the ensemble.
This pull request resolves this problem, if we delete one bookie with rack,
when notifying `rackawarePolicy`, the `bookieAddressList` contains the deleted
bookie and all other bookies, so `NetworkTopology` can hear the change(delete)
and remove bookie from rack. After this, this deleted bookie can't be selected
for a new ensemble
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMapping.java
##########
@@ -201,7 +216,16 @@ private void handleUpdates(Notification n) {
bookieAddressList.add(BookieId.parse(addr));
}
}
- rackawarePolicy.onBookieRackChange(bookieAddressList);
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Bookies with rack update from {} to
{}", bookieAddressListLastTime,
+ bookieAddressList);
+ }
+ if (bookieAddressListLastTime.size() >
bookieAddressList.size()) {
+
rackawarePolicy.onBookieRackChange(bookieAddressListLastTime);
Review comment:
The`handleUpdates` method will be called if there are any changes of
content under znode`/bookies`. So, if the contents are exactly the same, this
method will not be called.
The change of bookies with rack has three types: add (add a rack for a
bookie ), delete (delete an existing rack for a bookie), update (update rack
for a bookie).
Before this pull request, `handleUpdates` can only deal with add and update.
This is because if we delete rack for a bookie, the `bookieAddressList` to
notify `rackawarePolicy` does not contain the deleted bookie, hence the
`NetworkTopology` can't delete the rack of this deleted bookie. And then, this
deleted bookie can be still selected for the new ensemble.
This pull request resolves this problem, if we delete rack for a bookie,
when notifying `rackawarePolicy`, the `bookieAddressList` contains the deleted
bookie, so `NetworkTopology` can hear the change(delete) and remove bookie from
rack. After this, this deleted bookie can't be selected for a new ensemble
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
