kfaraz commented on code in PR #18228:
URL: https://github.com/apache/druid/pull/18228#discussion_r2197179151
##########
services/src/test/java/org/apache/druid/testing/embedded/EmbeddedDruidCluster.java:
##########
@@ -166,6 +167,14 @@ public EmbeddedDruidCluster addExtension(Class<? extends
DruidModule> moduleClas
return this;
}
+ @SafeVarargs
+ public final EmbeddedDruidCluster addExtensions(Class<? extends
DruidModule>... moduleClasses)
+ {
+ validateNotStarted();
+ extensionModules.addAll(Arrays.asList(moduleClasses));
Review Comment:
Nit: reads a little better
```suggestion
extensionModules.addAll(List.of(moduleClasses));
```
##########
services/src/test/java/org/apache/druid/testing/embedded/EmbeddedResource.java:
##########
@@ -39,6 +40,17 @@ public interface EmbeddedResource
*/
void stop() throws Exception;
+
+ /**
+ * Called before {@link #start()} with a pointer to the current cluster.
This is primarily useful for any
Review Comment:
Sounds more Java-ey?
```suggestion
* Called before {@link #start()} with a reference to the current cluster.
This is primarily useful for any
```
##########
services/src/test/java/org/apache/druid/testing/embedded/EmbeddedDruidServer.java:
##########
@@ -110,6 +150,12 @@ public final T addProperty(String key, String value)
return (T) this;
}
+ public final T addBeforeStart(BeforeStart hook)
Review Comment:
Nit: On the name, I feel we should either call it `doBeforeStart` or
`addBeforeStartHook`.
##########
services/src/test/java/org/apache/druid/testing/embedded/EmbeddedDruidServer.java:
##########
@@ -90,6 +122,14 @@ public void stop() throws Exception
}
}
+ @Override
+ public void beforeStart(EmbeddedDruidCluster cluster)
Review Comment:
Should this method be marked `final`?
##########
services/src/test/java/org/apache/druid/testing/embedded/EmbeddedDruidServer.java:
##########
@@ -110,6 +150,12 @@ public final T addProperty(String key, String value)
return (T) this;
}
+ public final T addBeforeStart(BeforeStart hook)
Review Comment:
Please add a short javadoc to this. It can just link to
`EmbeddedResource.beforeStart()`.
##########
services/src/test/java/org/apache/druid/testing/embedded/EmbeddedDruidCluster.java:
##########
@@ -166,6 +167,14 @@ public EmbeddedDruidCluster addExtension(Class<? extends
DruidModule> moduleClas
return this;
}
+ @SafeVarargs
+ public final EmbeddedDruidCluster addExtensions(Class<? extends
DruidModule>... moduleClasses)
Review Comment:
Please add a javadoc here that links to `{@link #addExtension}`.
##########
embedded-tests/src/test/java/org/apache/druid/testing/embedded/msq/EmbeddedDurableShuffleStorageTest.java:
##########
@@ -122,19 +124,25 @@ public EmbeddedDruidCluster createCluster()
.addServer(router);
}
- @BeforeEach
- void setUpEach()
+ @BeforeAll
Review Comment:
I am not sure if this `@BeforeAll` method will be run before or after the
one defined in `EmbeddedClusterTestBase`, or if the order is even deterministic.
Instead of this, can we just override the `setup` method and do the
initialization there?
##########
services/src/test/java/org/apache/druid/testing/embedded/EmbeddedHistorical.java:
##########
@@ -35,6 +36,20 @@
*/
public class EmbeddedHistorical extends EmbeddedDruidServer<EmbeddedHistorical>
{
+ public EmbeddedHistorical()
+ {
+ super();
Review Comment:
Nit: Is this explicit call needed?
##########
services/src/test/java/org/apache/druid/testing/embedded/EmbeddedServerLifecycle.java:
##########
@@ -151,7 +151,7 @@ private void runServer(ServerRunnable runnable)
try {
final Properties serverProperties = new Properties();
serverProperties.putAll(commonProperties);
- serverProperties.putAll(server.getStartupProperties(testFolder,
zookeeper));
+ serverProperties.putAll(server.getStartupProperties());
Review Comment:
Since this was the only usage of `testFolder` and `zookeeper`, we can remove
them from the constructor of `EmbeddedServerLifecycle`.
--
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]
