You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by "horizonzy (via GitHub)" <gi...@apache.org> on 2023/07/03 07:56:24 UTC
[GitHub] [bookkeeper] horizonzy commented on a diff in pull request #4009: Fix replace result adhere result wrong problem.
horizonzy commented on code in PR #4009:
URL: https://github.com/apache/bookkeeper/pull/4009#discussion_r1250430497
##########
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:
For getReplacementBookiesByIndexes, this change doesn't influence the result. https://github.com/apache/bookkeeper/blob/52e780f326c6cb5f303b51aa6ab5363c8e94b074/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java#L1088-L1110
Line_1109 already add the new bookie into `bookiesToExclude`.
It just didn't update the new bookie to the ensemble. It's different to test for `getReplacementBookiesByIndexes `, so I add a test to mock the internal logic in the `getReplacementBookiesByIndexes`.
--
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