hangc0276 commented on code in PR #4009:
URL: https://github.com/apache/bookkeeper/pull/4009#discussion_r1250414614


##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestRackawareEnsemblePlacementPolicy.java:
##########
@@ -636,6 +638,75 @@ public void testIsEnsembleAdheringToPlacementPolicy() 
throws Exception {
                 repp.isEnsembleAdheringToPlacementPolicy(ensemble, 3, 3));
     }
 
+    @Test
+    public void testReplaceBookieWithNewEnsemble() throws Exception {
+        BookieSocketAddress addr1 = new BookieSocketAddress("127.0.0.1", 3181);
+        BookieSocketAddress addr2 = new BookieSocketAddress("127.0.0.2", 3181);
+        BookieSocketAddress addr3 = new BookieSocketAddress("127.0.0.3", 3181);
+        BookieSocketAddress addr4 = new BookieSocketAddress("127.0.0.4", 3181);
+        BookieSocketAddress addr5 = new BookieSocketAddress("127.0.0.5", 3181);
+        // update dns mapping
+        StaticDNSResolver.addNodeToRack(addr1.getHostName(), 
"/default-region/r1");
+        StaticDNSResolver.addNodeToRack(addr2.getHostName(), 
"/default-region/r1");
+        StaticDNSResolver.addNodeToRack(addr3.getHostName(), 
"/default-region/r1");
+        StaticDNSResolver.addNodeToRack(addr4.getHostName(), 
"/default-region/r2");
+        StaticDNSResolver.addNodeToRack(addr5.getHostName(), 
"/default-region/r3");
+
+        ClientConfiguration conf = (ClientConfiguration) this.conf.clone();
+        conf.setMinNumRacksPerWriteQuorum(3);
+
+        repp = new RackawareEnsemblePlacementPolicy();
+        repp.initialize(conf, Optional.<DNSToSwitchMapping> empty(), timer, 
DISABLE_ALL,
+                NullStatsLogger.INSTANCE, 
BookieSocketAddress.LEGACY_BOOKIEID_RESOLVER);
+        repp.withDefaultRack(NetworkTopology.DEFAULT_REGION_AND_RACK);
+
+        // Update cluster
+        Set<BookieId> addrs = new HashSet<BookieId>();
+        addrs.add(addr1.toBookieId());
+        addrs.add(addr2.toBookieId());
+        addrs.add(addr3.toBookieId());
+        addrs.add(addr4.toBookieId());
+        addrs.add(addr5.toBookieId());
+        repp.onClusterChanged(addrs, new HashSet<>());
+
+        Set<Integer> bookieIndexesToRereplicate = new HashSet<>();
+        bookieIndexesToRereplicate.add(1);
+        bookieIndexesToRereplicate.add(2);
+
+        List<BookieId> ensemble = new ArrayList<>();
+        ensemble.add(addr1.toBookieId());
+        ensemble.add(addr2.toBookieId());
+        ensemble.add(addr3.toBookieId());
+
+        Set<BookieId> bookiesToExclude = Sets.newHashSet();
+
+        for (Integer bookieIndex : bookieIndexesToRereplicate) {
+            BookieId bookie = ensemble.get(bookieIndex);
+            bookiesToExclude.add(bookie);
+        }
+
+        List<BookieId> newEnsemble = new ArrayList<>(ensemble);
+
+        int i = 0;
+        for (Integer bookieIndex : bookieIndexesToRereplicate) {

Review Comment:
   Can you check the logic by calling getReplacementBookiesByIndexes directly 
or indirectly? By copy this changed logic to the test can't protect the 
original code logic.



-- 
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]

Reply via email to