Repository navigation
Conversation
40e4441 to
94971d1
Compare
|
@rmetzger Can you check this. Follow up to an ask on earlier PR. |
|
@Dennis-Mircea Can you help me a review. |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The broker-backed, separate-JVM integration setup warrants final human review.
Review effort: Balanced
Findings: None
What changed in this PR
Adds broker-backed integration coverage for Kafka/Pulsar source partition-metric detection using Testcontainers and an isolated Flink MiniCluster JVM.
Changes:
- Adds Kafka, Pulsar, and non-broker source integration scenarios.
- Adds a separate-JVM MiniCluster test driver.
- Adds test dependencies, Failsafe configuration, and CI execution.
| File | Description |
|---|---|
flink-autoscaler/src/test/java/org/apache/flink/autoscaler/MiniClusterJobDriver.java |
Runs test jobs in an isolated MiniCluster JVM. |
flink-autoscaler/src/test/java/org/apache/flink/autoscaler/KafkaPulsarPartitionMetricsITCase.java |
Verifies partition detection against real Kafka and Pulsar brokers. |
flink-autoscaler/pom.xml |
Adds test dependencies and Failsafe configuration. |
.github/workflows/ci.yml |
Runs autoscaler integration tests in CI. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Files.deleteIfExists(driverResultFile); | ||
| } | ||
| if (driverLogFile != null) { | ||
| Files.deleteIfExists(driverLogFile); |
There was a problem hiding this comment.
this will delete the log also in case of a failure -- keep it so that debugging is easier.
There was a problem hiding this comment.
ideally when there's an error, print the entire log contents to the output?
There was a problem hiding this comment.
Done in 338e8ce — the driver log is now kept when setup or the test fails, and its full contents are logged to the test output (CI doesn't upload artifacts, so the console is where it'll be seen). It's only deleted after a successful run.
| /** | ||
| * Polls {@code resultFile} for the two lines {@link MiniClusterJobDriver} writes once ready. | ||
| */ | ||
| private static void awaitDriverReady(Process process, Path resultFile) throws Exception { |
There was a problem hiding this comment.
use org.awaitility.Awaitility here as well?
There was a problem hiding this comment.
Done in 338e8ce — switched awaitDriverReady to Awaitility; an early driver exit still fails immediately.
|
I agree with the approach and direction in this PR, happy to merge it once above items are addressed. |
…ion detection Follow-up to FLINK-40443/PR apache#1187 per maintainer consensus: verifies ScalingMetricCollector derives the correct numSourcePartitions against real Kafka and Pulsar brokers (multi-topic summing, dotted/hyphenated topic names, parallelism > 1, multiple source vertices, and a non-Kafka/Pulsar negative case), without touching the hardening fix itself. Generated-by: Claude Code
Adds the same per-module "verify -Dit.skip=false" step already used for flink-kubernetes-operator, flink-kubernetes-webhook, and flink-autoscaler-plugin-jdbc, so the new Kafka/Pulsar autoscaler ITCase actually runs instead of being silently skipped by the whole-reactor build. Generated-by: Claude Code
…y for driver readiness Generated-by: Claude Code (Claude Opus 5.5)
338e8ce to
7df0b66
Compare
What is the purpose of the change
This pull request adds an integration test that verifies
ScalingMetricCollectorcorrectly derivesnumSourcePartitionsfor Kafka and Pulsar sources against real brokers. It is a follow-up to FLINK-40443 / #1187, added per maintainer consensus on that PR as a separate, non-blocking PR. It does not touch the hardening fix itself.Brief change log
KafkaPulsarPartitionMetricsITCase, covering: multi-topic summing, dotted and hyphenated topic names, source parallelism > 1, multiple source vertices in one job, and a non-Kafka/Pulsar source that must not report a partition countMiniClusterJobDriver, which runs the Flink job under test via aMiniClusterin its own JVM rather than in-process, becauseflink-autoscalervendors a compatibility copy oforg.apache.flink.runtime.rest.messages.job.JobDetailsInfoundersrc/main/javathat shadows the real Flink class on this module's own classpath, which breaks a same-process JobManager's own REST handlersflink-autoscaler/pom.xml(flink-connector-kafka,flink-connector-pulsar,testcontainers-kafka,testcontainers-pulsar,commons-codec) and disables the failsafe module path (useModulePath=false), since the connector jars otherwise get placed on the module path as automatic modules and lose visibility intoflink-runtime's unnamed-module classpath classesflink-autoscalerinto CI (ci.yml) with the same per-moduleverify -Dit.skip=falsestep already used forflink-kubernetes-operator,flink-kubernetes-webhook, andflink-autoscaler-plugin-jdbc, so the new ITCase actually runs instead of being silently skipped by the whole-reactor buildVerifying this change
This change added tests and can be verified as follows:
KafkaPulsarPartitionMetricsITCase, an integration test that spins up real Kafka and Pulsar brokers via Testcontainers and assertsScalingMetricCollector#getJobTopologyreports the correctnumSourcePartitionsfor: multi-topic summing, dotted/hyphenated topic names, source parallelism > 1, multiple source vertices in one job, and a negative case (non-Kafka/Pulsar source reports no partition count)mvn -pl flink-autoscaler verify -Dit.skip=falsepasses locally (300 unit tests + the new IT)mvn -pl flink-autoscaler checkstyle:check spotless:checkis cleanTests in flink-autoscalerstep: https://github.com/apache/flink-kubernetes-operator/actions/runs/32391143762Does this pull request potentially affect one of the following parts:
flink-autoscaler/pom.xml(flink-connector-kafka,flink-connector-pulsar,testcontainers-kafka,testcontainers-pulsar,commons-codec)CustomResourceDescriptors: noDocumentation
Was generative AI tooling used to co-author this PR?