jtuglu1 commented on code in PR #19787:
URL: https://github.com/apache/druid/pull/19787#discussion_r3670566211
##########
server/src/test/java/org/apache/druid/client/BrokerViewOfCoordinatorConfigTest.java:
##########
@@ -55,4 +61,90 @@ public void testFetchesConfigOnStartup()
Mockito.verify(coordinatorClient,
Mockito.times(1)).getCoordinatorDynamicConfig();
Assert.assertEquals(config, target.getDynamicConfig());
}
+
+ @Test
+ public void testExcludeClonesFiltersTargetCloneServers()
+ {
+ target.start();
+ Int2ObjectRBTreeMap<Set<QueryableDruidServer>> servers =
makeServers("host1", "host2", "host3");
+
+ Int2ObjectRBTreeMap<Set<QueryableDruidServer>> result =
+ target.getQueryableServers(servers, CloneQueryMode.EXCLUDECLONES);
+
+ Set<String> hosts = extractHosts(result);
+ Assert.assertFalse("target clone server host1 should be filtered",
hosts.contains("host1"));
+ Assert.assertTrue("source clone server host2 should remain",
hosts.contains("host2"));
+ Assert.assertTrue("non-clone server host3 should remain",
hosts.contains("host3"));
+ }
+
+ @Test
+ public void testPreferClonesFiltersSourceCloneServers()
+ {
+ target.start();
+ Int2ObjectRBTreeMap<Set<QueryableDruidServer>> servers =
makeServers("host1", "host2", "host3");
+
+ Int2ObjectRBTreeMap<Set<QueryableDruidServer>> result =
+ target.getQueryableServers(servers, CloneQueryMode.PREFERCLONES);
+
+ Set<String> hosts = extractHosts(result);
+ Assert.assertTrue("target clone server host1 should remain",
hosts.contains("host1"));
+ Assert.assertFalse("source clone server host2 should be filtered",
hosts.contains("host2"));
+ Assert.assertTrue("non-clone server host3 should remain",
hosts.contains("host3"));
+ }
+
+ @Test
+ public void testIncludeClonesReturnsAll()
+ {
+ target.start();
+ Int2ObjectRBTreeMap<Set<QueryableDruidServer>> servers =
makeServers("host1", "host2", "host3");
+
+ Int2ObjectRBTreeMap<Set<QueryableDruidServer>> result =
+ target.getQueryableServers(servers, CloneQueryMode.INCLUDECLONES);
+
+ Assert.assertSame("INCLUDECLONES should return the original map", servers,
result);
+ }
+
+ @Test
+ public void testConfigUpdateChangesFiltering()
+ {
+ target.start();
+
+ CoordinatorDynamicConfig newConfig = CoordinatorDynamicConfig.builder()
+
.withCloneServers(Map.of("host3", "host1"))
+ .build();
+ target.setDynamicConfig(newConfig);
+
+ Int2ObjectRBTreeMap<Set<QueryableDruidServer>> servers =
makeServers("host1", "host2", "host3");
+
+ Int2ObjectRBTreeMap<Set<QueryableDruidServer>> result =
+ target.getQueryableServers(servers, CloneQueryMode.EXCLUDECLONES);
+
+ Set<String> hosts = extractHosts(result);
+ Assert.assertFalse("new target clone host3 should be filtered",
hosts.contains("host3"));
+ Assert.assertTrue("host1 is now source, should remain",
hosts.contains("host1"));
+ Assert.assertTrue("host2 is unrelated, should remain",
hosts.contains("host2"));
+ }
+
+ private static Int2ObjectRBTreeMap<Set<QueryableDruidServer>>
makeServers(String... hosts)
Review Comment:
nit: javadoc
##########
server/src/main/java/org/apache/druid/client/BrokerViewOfCoordinatorConfig.java:
##########
@@ -54,10 +53,11 @@ public class BrokerViewOfCoordinatorConfig extends
BaseBrokerViewOfConfig<Coordi
{
private final CoordinatorClient coordinatorClient;
- @GuardedBy("this")
- private Set<String> targetCloneServers;
- @GuardedBy("this")
- private Set<String> sourceCloneServers;
+ // volatile, not synchronized: getCurrentServersToIgnore() is called
per-segment during query planning.
+ // Under high concurrency, synchronized causes monitor convoy with 100x
throughput degradation.
+ // Each field is an immutable Set reference, so volatile provides sufficient
visibility.
+ private volatile Set<String> targetCloneServers = Set.of();
Review Comment:
This does remove synchronization across the 2 sets, which could cause
(hopefully benign) races in update order like seeing target server before the
corresponding source, etc.
##########
server/src/test/java/org/apache/druid/client/BrokerViewOfCoordinatorConfigTest.java:
##########
@@ -55,4 +61,90 @@ public void testFetchesConfigOnStartup()
Mockito.verify(coordinatorClient,
Mockito.times(1)).getCoordinatorDynamicConfig();
Assert.assertEquals(config, target.getDynamicConfig());
}
+
+ @Test
+ public void testExcludeClonesFiltersTargetCloneServers()
+ {
+ target.start();
+ Int2ObjectRBTreeMap<Set<QueryableDruidServer>> servers =
makeServers("host1", "host2", "host3");
+
+ Int2ObjectRBTreeMap<Set<QueryableDruidServer>> result =
+ target.getQueryableServers(servers, CloneQueryMode.EXCLUDECLONES);
+
+ Set<String> hosts = extractHosts(result);
+ Assert.assertFalse("target clone server host1 should be filtered",
hosts.contains("host1"));
+ Assert.assertTrue("source clone server host2 should remain",
hosts.contains("host2"));
+ Assert.assertTrue("non-clone server host3 should remain",
hosts.contains("host3"));
+ }
+
+ @Test
+ public void testPreferClonesFiltersSourceCloneServers()
+ {
+ target.start();
+ Int2ObjectRBTreeMap<Set<QueryableDruidServer>> servers =
makeServers("host1", "host2", "host3");
+
+ Int2ObjectRBTreeMap<Set<QueryableDruidServer>> result =
+ target.getQueryableServers(servers, CloneQueryMode.PREFERCLONES);
+
+ Set<String> hosts = extractHosts(result);
+ Assert.assertTrue("target clone server host1 should remain",
hosts.contains("host1"));
+ Assert.assertFalse("source clone server host2 should be filtered",
hosts.contains("host2"));
+ Assert.assertTrue("non-clone server host3 should remain",
hosts.contains("host3"));
+ }
+
+ @Test
+ public void testIncludeClonesReturnsAll()
+ {
+ target.start();
+ Int2ObjectRBTreeMap<Set<QueryableDruidServer>> servers =
makeServers("host1", "host2", "host3");
+
+ Int2ObjectRBTreeMap<Set<QueryableDruidServer>> result =
+ target.getQueryableServers(servers, CloneQueryMode.INCLUDECLONES);
+
+ Assert.assertSame("INCLUDECLONES should return the original map", servers,
result);
+ }
+
+ @Test
+ public void testConfigUpdateChangesFiltering()
+ {
+ target.start();
+
+ CoordinatorDynamicConfig newConfig = CoordinatorDynamicConfig.builder()
+
.withCloneServers(Map.of("host3", "host1"))
+ .build();
+ target.setDynamicConfig(newConfig);
+
+ Int2ObjectRBTreeMap<Set<QueryableDruidServer>> servers =
makeServers("host1", "host2", "host3");
+
+ Int2ObjectRBTreeMap<Set<QueryableDruidServer>> result =
+ target.getQueryableServers(servers, CloneQueryMode.EXCLUDECLONES);
+
+ Set<String> hosts = extractHosts(result);
+ Assert.assertFalse("new target clone host3 should be filtered",
hosts.contains("host3"));
+ Assert.assertTrue("host1 is now source, should remain",
hosts.contains("host1"));
+ Assert.assertTrue("host2 is unrelated, should remain",
hosts.contains("host2"));
+ }
+
+ private static Int2ObjectRBTreeMap<Set<QueryableDruidServer>>
makeServers(String... hosts)
+ {
+ Int2ObjectRBTreeMap<Set<QueryableDruidServer>> map = new
Int2ObjectRBTreeMap<>();
+ Set<QueryableDruidServer> serverSet = new HashSet<>();
+ for (String host : hosts) {
+ DruidServer druidServer = new DruidServer(host, host, null, 100, null,
ServerType.HISTORICAL, "tier1", 0);
+ serverSet.add(new QueryableDruidServer(druidServer,
Mockito.mock(QueryRunner.class)));
+ }
+ map.put(0, serverSet);
+ return map;
+ }
+
+ private static Set<String>
extractHosts(Int2ObjectRBTreeMap<Set<QueryableDruidServer>> servers)
Review Comment:
nit: javadoc
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
