Repository navigation
Conversation
|
/gemini review |
There was a problem hiding this comment.
📋 Review Summary
This PR stabilizes the PerformanceTests SparkReceiver IO workflow by waiting for the RabbitMQ StatefulSet rollout before starting integration tests and relaxing the RabbitMQ container probes and storage sizing under heavy write loads. The changes directly address the race conditions during service provisioning and premature container restarts during bulk publishing.
🔍 General Feedback
- Adding
kubectl rollout status statefulset/rabbitmqproperly ensuresrabbitmq-0is ready before the test attempts to connect to the LoadBalancer IP. - Increasing the PVC size to
10Giand relaxing thelivenessProbeandreadinessProbetimeouts/intervals onrabbitmqctl statusare sensible adjustments for handling the 5M message bulk publish phase.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
…OIT.writeToRabbitMq
|
/gemini review |
There was a problem hiding this comment.
📋 Review Summary
This PR stabilizes the PerformanceTests SparkReceiver IO integration test by ensuring the RabbitMQ StatefulSet rollout and LoadBalancer TCP endpoint are fully ready before running tests, relaxing liveness/readiness probe timeouts and increasing PVC storage to avoid mid-test pod restarts under heavy write load, and adding connection retry logic in SparkReceiverIOIT.writeToRabbitMq.
🔍 General Feedback
- The Kubernetes readiness sequence, probe configuration adjustments, and retry logic with stream cleanup directly address the root causes of the observed connection timeouts and mid-publish resets.
- Consider disabling automatic connection recovery on the
ConnectionFactoryinsidewriteToRabbitMqso that manual retry iterations cannot race with background auto-recovery threads.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
| final ConnectionFactory connectionFactory = new ConnectionFactory(); | ||
| connectionFactory.setUri(options.getRabbitMqBootstrapServerAddress()); | ||
| connectionFactory.setConnectionTimeout(30000); | ||
| connectionFactory.setAutomaticRecoveryEnabled(true); |
There was a problem hiding this comment.
🟢 Nit: setAutomaticRecoveryEnabled(true) is already enabled by default in com.rabbitmq.client.ConnectionFactory, and having automatic background recovery active alongside the explicit for loop retry + clearRabbitMQ() (which deletes the stream) could allow a background recovery thread to reconnect on a connection before the try-with-resources block finishes closing it. Explicitly disabling automatic recovery (false) ensures clean, deterministic tear-down and recreation across attempts.
| connectionFactory.setAutomaticRecoveryEnabled(true); | |
| connectionFactory.setAutomaticRecoveryEnabled(false); |
|
/gemini review |
There was a problem hiding this comment.
📋 Review Summary
This PR stabilizes the SparkReceiverIOIT performance test by waiting for the RabbitMQ StatefulSet rollout and TCP port readiness before running tests, relaxing livenessProbe and readinessProbe timeouts, increasing the PVC request size, and adding connection retry logic during bulk message publishing.
🔍 General Feedback
- The Kubernetes probe adjustments, StatefulSet readiness checks, and Java client retry mechanism directly address the root causes of the test flakiness under heavy write loads and look clean and well-structured.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
Summary
Fixes flaky failures in
PerformanceTests SparkReceiver IO(:sdks:java:io:sparkreceiver:3:integrationTestSparkReceiverIOIT.testSparkReceiverIOReadsInStreamingWithOffset, e.g. https://github.com/apache/beam/actions/runs/42310450785, https://github.com/apache/beam/actions/runs/40188354039, https://github.com/apache/beam/actions/runs/39692762153):Install rabbitmqinbeam_PerformanceTests_SparkReceiver_IO.ymlonly waited forsvc/rabbitmqto receive a LoadBalancer IP (kubectl wait svc/rabbitmq --for=jsonpath='{.status.loadBalancer.ingress[0].ip}') without waiting for thestatefulset/rabbitmqpod (rabbitmq-0) to become Ready, causingSocketTimeoutException: Connect timed outwhen the LoadBalancer IP was assigned before the pod finished starting.rabbitmqctl statusprobe timeouts and increase PVC size: DuringSparkReceiverIOIT.writeToRabbitMq, the test publishes 5,000,000 persistent messages in a tight loop. Runningrabbitmqctl statusevery 10s with a 10s liveness/readiness timeout on a 3Gi standard PD causedlivenessProbetimeouts under heavy write load, causing kubelet to kill and restartrabbitmq-0mid-publish (SocketException: Broken pipe/Connection reset). Increasing the probe timeout/period and PVC size prevents premature container restarts during bulk publishing.Verification
PerformanceTests SparkReceiver IOon this branch: https://github.com/apache/beam/actions/runs/42316569422