-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Add DruidContainer to completely phase out revised and standard ITs #18265
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The license check keeps failing. Need to fix up for reference: |
| Assertions.assertEquals(Map.of("id", supervisorId), startSupervisorResult); | ||
|
|
||
| // Wait for the broker to discover the realtime segments | ||
| broker2.latchableEmitter().waitForEvent( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious why does LatchableEmitter has waitForEvent(UnaryOperator<EventMatcher> condition), it seems like it can just do waitForEvent(EventMatcher matcher)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did have that initially but when I used the lamda, the tests became much more readable. So we kept this syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference is very slight but I preferred the second syntax:
waitForEvent(EventMatcher.matchesA(...).matchesB(...))and
waitForEvent(event -> event.matchesA().matchesB(...))|
|
||
| private KafkaSupervisorTuningConfig createTuningConfig() | ||
| { | ||
| return new KafkaSupervisorTuningConfig( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: might be more readable if we use a builder, or mapper.readValue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that makes sense. There's another PR that needs to be merged. It has some builders which will be used here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.github/workflows/docker-tests.yml
Outdated
| - name: Run the test | ||
| uses: ./.github/workflows/worker.yml | ||
| with: | ||
| script: .github/scripts/run_unit-tests -Dtest=*DockerTest -Ddruid.testing.docker.image=$DRUID_DIST_IMAGE_NAME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is DockerTest defined? is this some kind of smoke test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an IngestionDockerTest that is being newly added in this PR. This command will run future tests also which have the same suffix. But yes, for the most part, it is meant to be a smoke test.
|
Closing in favor of #18302 |
Changes
druid-testcontainersthat can be later published as an individual module forTestcontainersDruidContainer-Testcontainerimpl for running individual Druid servicesDruidContainerResourceto allow use ofDruidContainerin embedded cluster testsEmbeddedDruidServerandEmbeddedDruidClusterto supportrunning both embedded servers and container-based services in the same cluster
*DockerTests (2 right now, this number should remain small over time).Note: Usage of
DruidContainerwill not be the norm but the exception to test out backward compatibilityand/or Docker related changes only. All other use cases should continue to use
EmbeddedDruidServers.Pending items
Advantages
which use a custom IT-only image
Next steps
EmbeddedClusterTestBaseEmbeddedDruidServer(and required containers only, likeKafkaResource)DruidContainer, some of which will use MiddleManagersThis PR has: