[GitHub] [pulsar] eolivelli commented on a change in pull request #14248: fix rackaware placement policy does not take effect after delete rack configuration

[GitBox <gi...@apache.org>]

eolivelli commented on a change in pull request #14248:
URL: https://github.com/apache/pulsar/pull/14248#discussion_r805148276



##########
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:
       We should fail here, otherwise the behaviour won't be predictable 

##########
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:
       We should log something.
   This is not obvious to me.
   
   I could understand to not call the callback if the contents are exacly the same but here we are ignoring some changes

##########
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:
       This call is blocking and we have only one broker and one zookeeper server
   So there must be no need to use awaiatility below




-- 
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@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org