abhishekagarwal87 commented on code in PR #15726:
URL: https://github.com/apache/druid/pull/15726#discussion_r1512779921
##########
server/src/main/java/org/apache/druid/discovery/BaseNodeRoleWatcher.java:
##########
@@ -264,8 +267,15 @@ private void cacheInitializedTimedOut()
@GuardedBy("lock")
private void cacheInitialized(boolean timedOut)
{
+ // This method is called only once with either timedOut = true or false,
but not both.
Review Comment:
nit: usually such comments are meant to be placed above the method and not
inside.
##########
server/src/test/java/org/apache/druid/discovery/BaseNodeRoleWatcherTest.java:
##########
@@ -190,41 +196,39 @@ private DiscoveryDruidNode
buildDiscoveryDruidNode(NodeRole role, String host)
);
}
- private void assertListenerTimedOut(TestListener listener)
- {
- try {
- exec.submit(() -> {
- Assert.assertTrue(listener.timedOut.get());
- }).get();
- }
- catch (Exception e) {
- Assert.fail();
- }
- }
-
private void assertListener(
TestListener listener,
boolean nodeViewInitialized,
List<DiscoveryDruidNode> nodesAdded,
List<DiscoveryDruidNode> nodesRemoved
)
{
- try {
- exec.submit(() -> {
- Assert.assertEquals(nodeViewInitialized,
listener.nodeViewInitialized.get());
- Assert.assertEquals(nodesAdded, listener.nodesAddedList);
- Assert.assertEquals(nodesRemoved, listener.nodesRemovedList);
- }).get();
- }
- catch (Exception e) {
- Assert.fail();
- }
+ Assert.assertEquals(nodeViewInitialized,
listener.nodeViewInitialized.get());
+ Assert.assertEquals(nodesAdded, listener.nodesAddedList);
+ Assert.assertEquals(nodesRemoved, listener.nodesRemovedList);
+ }
+
+ private ScheduledExecutorService createScheduledSingleThreadedExecutor()
+ {
+ return new ScheduledThreadPoolExecutor(
+ 1,
+ Execs.makeThreadFactory("BaseNodeRoleWatcher")
+ )
+ {
+ @Override
+ public Future<?> submit(Runnable task)
+ {
+ task.run();
+ return Futures.immediateFuture(null);
+ }
+ };
}
public static class TestListener implements DruidNodeDiscovery.Listener
{
- private final AtomicBoolean timedOut = new AtomicBoolean(false);
+ private final CountDownLatch ready = new CountDownLatch(1);
private final AtomicBoolean nodeViewInitialized = new AtomicBoolean(false);
+ private final AtomicBoolean nodeViewInitializationTimedOut = new
AtomicBoolean(false);
Review Comment:
do you need two flags? Because only one thing should happen, either a
timeout or a proper initialization
##########
server/src/test/java/org/apache/druid/discovery/BaseNodeRoleWatcherTest.java:
##########
@@ -141,17 +146,18 @@ public void testRegisterListenerBeforeTimeout() throws
InterruptedException
nodeRoleWatcher.childRemoved(broker2);
assertListener(listener1, false, Collections.emptyList(),
Collections.emptyList());
+ Assert.assertFalse(listener1.nodeViewInitializationTimedOut.get());
Review Comment:
I think this can be removed.
##########
server/src/test/java/org/apache/druid/discovery/BaseNodeRoleWatcherTest.java:
##########
@@ -141,17 +146,18 @@ public void testRegisterListenerBeforeTimeout() throws
InterruptedException
nodeRoleWatcher.childRemoved(broker2);
assertListener(listener1, false, Collections.emptyList(),
Collections.emptyList());
+ Assert.assertFalse(listener1.nodeViewInitializationTimedOut.get());
Review Comment:
In general, this assert too relies on timing of events. something we should
avoid if we can.
--
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]
