some enhancements to embedded tests for writing query oriented tests #18228
Conversation
kfaraz
left a comment
kfaraz
left a comment
There was a problem hiding this comment.
Looks good! 🚀
The startup properties flow seems nicer now.
| public final EmbeddedDruidCluster addExtensions(Class<? extends DruidModule>... moduleClasses) | ||
| { | ||
| validateNotStarted(); | ||
| extensionModules.addAll(Arrays.asList(moduleClasses)); |
There was a problem hiding this comment.
Nit: reads a little better
| extensionModules.addAll(Arrays.asList(moduleClasses)); | |
| extensionModules.addAll(List.of(moduleClasses)); |
services/src/test/java/org/apache/druid/testing/embedded/EmbeddedResource.java
Show resolved
Hide resolved
services/src/test/java/org/apache/druid/testing/embedded/EmbeddedServerLifecycle.java
Show resolved
Hide resolved
...s/src/test/java/org/apache/druid/testing/embedded/msq/EmbeddedDurableShuffleStorageTest.java
Show resolved
Hide resolved
services/src/test/java/org/apache/druid/testing/embedded/EmbeddedDruidCluster.java
Show resolved
Hide resolved
| return (T) this; | ||
| } | ||
|
|
||
| public final T addBeforeStart(BeforeStart hook) |
There was a problem hiding this comment.
Please add a short javadoc to this. It can just link to EmbeddedResource.beforeStart().
services/src/test/java/org/apache/druid/testing/embedded/EmbeddedDruidServer.java
Show resolved
Hide resolved
| { | ||
| public EmbeddedHistorical() | ||
| { | ||
| super(); |
There was a problem hiding this comment.
Nit: Is this explicit call needed?
| return (T) this; | ||
| } | ||
|
|
||
| public final T addBeforeStart(BeforeStart hook) |
There was a problem hiding this comment.
Nit: On the name, I feel we should either call it doBeforeStart or addBeforeStartHook.
| logsDirectory, | ||
| storageDirectory | ||
| ); | ||
| self.addProperty("druid.host", "localhost"); |
There was a problem hiding this comment.
Just curious, did you run into any issue with this not being hardcoded here?
There was a problem hiding this comment.
not currently, but i have had problems with stuff like this in the past so was proactively changing it. The problem i was running into was that some networks DNS services try to be "helpful" and redirect to a search page or the like, so what should be an unresolvable local network hostname ends up being a real address that is definitely not localhost when using InetAddress.getLocalHost().getCanonicalHostName().
There was a problem hiding this comment.
I see. That makes sense.
I am currently working on something where I think I might need the non-localhost hostname.
But I will see what I can do to avoid that requirement as it is cleaner to just have it be localhost.
| /** | ||
| * Adds an extension to this cluster. The list of extensions is populated in | ||
| * the common property {@code druid.extensions.modulesForSimulation}. | ||
| * the common property {@code druid.extensions.modulesForEmbeddedTest}. |
|
UT failure seems unrelated to the change here, @uds5501 is taking a look. |
3 participants
Some improvements to the embedded test stuff from my experience writing test for #18176, which is a query oriented test.
The primary change is a new
beforeStartmethod onEmbeddedResourceinterface, which allows them to perform additional operations when starting up (with access to theEmbeddedDruidClusterat this point, so shared resources are available).I have added usage of this new functionality to clean up how druid service properties are set, specifically
EmbeddedDruidServicehas a newaddBeforeStartmethod that takes a function add to a list of things to call during its implementation of the newbeforeStartmethod. With this, I've cleaned up thegetStartupPropertiesmethod to just copy all the stuff from the map, and moved all of the previous logic into thebeforeStarthooks (and the historical specific logic into theEmbeddedHistorical).While i was here, I made the durable shuffle test re-use the same ingested data between runs instead of reingesting per test.