You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "schizophrenicmaniac (via GitHub)" <gi...@apache.org> on 2026/10/23 00:19:16 UTC
[PR] [Solace] Fix flaky SolaceIO integration tests [beam]
schizophrenicmaniac opened a new pull request, #40740:
URL: https://github.com/apache/beam/pull/40740
Fixes #40734
The Solace IO Direct precommit fails often because of how the integration tests are set up. This PR changes three things.
The container used fixed host ports from `findAvailablePort()`. That opens a socket on port 0, closes it, and only binds the port when the container starts, so something else can take the port in between. The ports are now normally exposed, and the mapped ports are read with `getMappedPort` after the container starts.
The read tests waited a fixed 15 seconds and then checked the counter. On a slow runner that can be too short, and since the read never finishes, the pipeline was left running in the test JVM afterwards. A small helper now polls the read counter until it reaches the expected count, with a 2 minute limit, and then cancels the pipeline. This applies to `SolaceIOIT`, `SolaceIOMultipleSempIT` and `SolaceIOCustomSessionServiceFactoryIT`.
Creating the queue and its subscription through SEMP only logged errors, and curl exits 0 on HTTP errors anyway. If SEMP wasn't ready yet, the setup kept going and the read test later failed with a wrong count. Those two calls now use `curl --fail`, retry for up to 30 seconds, and fail the setup with a clear error if they still don't succeed.
These are test-only changes. I couldn't run the ITs locally since I don't have Docker on this machine, so I'm relying on this PR's precommit run, ideally re-run a few times. Unit tests, checkstyle and spotbugs pass for the module.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Solace] Fix flaky SolaceIO integration tests [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40740:
URL: https://github.com/apache/beam/pull/40740#issuecomment-6355166249
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment `assign set of reviewers`
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Solace] Fix flaky SolaceIO integration tests [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40740:
URL: https://github.com/apache/beam/pull/40740#issuecomment-6359730504
Assigning reviewers:
R: @Abacn for label java.
Backup reviewer: kennknowles (if the main reviewer is unresponsive, you can escalate to them by tagging their username)
Note: If you would like to opt out of this review, comment `assign to next reviewer`.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Solace] Fix flaky SolaceIO integration tests [beam]
Posted by "schizophrenicmaniac (via GitHub)" <gi...@apache.org>.
schizophrenicmaniac commented on PR #40740:
URL: https://github.com/apache/beam/pull/40740#issuecomment-6450261965
@aIbrahiim You're right, the job was actually broken by the `Unsupported class file major version 69` error. Master now has the same java-version pin (plus the settings.xml step), and I merged master and resolved the conflict in the workflow file, so the pin is no longer part of this PR's diff.
The test changes weren't the cause of the failures in those logs. They're for separate flakiness risks I noticed in the IT setup. `findAvailablePort()` reserves host ports and releases them before the container binds them, so another process can take them in between. The read tests sleep a fixed 15s before checking the counter and never cancel the unbounded read pipeline. And the SEMP queue/subscription setup ignores errors, so if SEMP isn't ready yet the test only fails later with a wrong count.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Solace] Fix flaky SolaceIO integration tests [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #40740:
URL: https://github.com/apache/beam/pull/40740#discussion_r4354895487
##########
sdks/java/io/solace/src/test/java/org/apache/beam/sdk/io/solace/it/SolaceTestUtils.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.io.solace.it;
+
+import java.io.IOException;
+import org.apache.beam.sdk.PipelineResult;
+import org.apache.beam.sdk.testutils.metrics.MetricsReader;
+import org.joda.time.Duration;
+import org.joda.time.Instant;
+
+class SolaceTestUtils {
+ private static final Duration POLL_INTERVAL = Duration.standardSeconds(1);
+
+ /**
+ * Waits until the counter reaches {@code expectedCount} or {@code timeout} passes, then cancels
+ * the pipeline and returns the last value of the counter.
+ *
+ * <p>The Read connector keeps polling the queue forever, so the pipeline never finishes on its
+ * own. Waiting for the expected count instead of a fixed amount of time keeps the tests from
+ * stopping too early on a slow machine.
+ */
+ static long waitForCounterAndCancel(
+ PipelineResult pipelineResult,
+ String namespace,
+ String counterName,
+ long expectedCount,
+ Duration timeout)
+ throws IOException {
+ MetricsReader metricsReader = new MetricsReader(pipelineResult, namespace);
+ Instant deadline = Instant.now().plus(timeout);
+ long count = metricsReader.getCounterMetric(counterName);
+ while (count < expectedCount && Instant.now().isBefore(deadline)) {
+ PipelineResult.State state = pipelineResult.waitUntilFinish(POLL_INTERVAL);
+ count = metricsReader.getCounterMetric(counterName);
+ if (state != null && state.isTerminal()) {
+ break;
+ }
+ }
+ pipelineResult.cancel();
Review Comment:
**🟡 Medium**: If an exception is thrown inside the `while` loop (e.g., from `waitUntilFinish` or `getCounterMetric`), `pipelineResult.cancel()` will not be called, leaving the unbounded streaming pipeline running in the JVM. Consider wrapping the polling loop in a `try ... finally` block so the pipeline is always cancelled if it hasn't reached a terminal state.
##########
sdks/java/io/solace/src/test/java/org/apache/beam/sdk/io/solace/it/SolaceContainerManager.java:
##########
@@ -37,25 +36,29 @@ public class SolaceContainerManager {
public static final String USERNAME = "username";
public static final String TOPIC_NAME = "test_topic";
private static final Logger LOG = LoggerFactory.getLogger(SolaceContainerManager.class);
+ private static final int JCSMP_PORT = 55555;
+ private static final int SEMP_PORT = 8080;
+ private static final int MAX_COMMAND_ATTEMPTS = 30;
private final SolaceContainer container;
- int jcsmpPortMapped = findAvailablePort();
- int sempPortMapped = findAvailablePort();
+ int jcsmpPortMapped;
+ int sempPortMapped;
- public SolaceContainerManager() throws IOException {
+ public SolaceContainerManager() {
+ // Let Docker pick the host ports. Reserving a free port up front and binding it later races
+ // with anything else that grabs the port in between.
this.container =
- new SolaceContainer(DockerImageName.parse("solace/solace-pubsub-standard:10.7")) {
- {
- addFixedExposedPort(jcsmpPortMapped, 55555);
- addFixedExposedPort(sempPortMapped, 8080);
- }
- }.withVpn(VPN_NAME)
+ new SolaceContainer(DockerImageName.parse("solace/solace-pubsub-standard:10.7"))
+ .withVpn(VPN_NAME)
.withCredentials(USERNAME, PASSWORD)
.withTopic(TOPIC_NAME, Service.SMF)
+ .withExposedPorts(SEMP_PORT, JCSMP_PORT)
Review Comment:
**🟢 Low**: `GenericContainer.withExposedPorts(...)` overwrites the container's exposed ports list rather than appending to it, which replaces the port added by `.withTopic(TOPIC_NAME, Service.SMF)` on the line above. While it works here because `JCSMP_PORT` (55555) is the same as `Service.SMF.getPort()`, `SolaceContainer` already exposes port 8080 (`SEMP_PORT`) in its constructor and `withTopic(..., Service.SMF)` already calls `addExposedPort(55555)`, so `.withExposedPorts(SEMP_PORT, JCSMP_PORT)` is redundant (or `addExposedPorts` could be used if you want to be explicit without overwriting).
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Solace] Fix flaky SolaceIO integration tests [beam]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #40740:
URL: https://github.com/apache/beam/pull/40740#issuecomment-6359744543
## [Codecov](https://app.codecov.io/gh/apache/beam/pull/40740?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 0.00%. Comparing base ([`a8d6afd`](https://app.codecov.io/gh/apache/beam/commit/a8d6afdf5cbc8c4635732ad6dd8b702d16d869ec?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`84c3c5c`](https://app.codecov.io/gh/apache/beam/commit/84c3c5c9595fdafe34651be6c59dbbabaee75471?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 53 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #40740 +/- ##
===========================================
- Coverage 80.04% 0 -80.05%
===========================================
Files 490 0 -490
Lines 82916 0 -82916
===========================================
- Hits 66373 0 -66373
+ Misses 16543 0 -16543
```
| [Flag](https://app.codecov.io/gh/apache/beam/pull/40740/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [python](https://app.codecov.io/gh/apache/beam/pull/40740/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/beam/pull/40740?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Solace] Fix flaky SolaceIO integration tests [beam]
Posted by "aIbrahiim (via GitHub)" <gi...@apache.org>.
aIbrahiim commented on PR #40740:
URL: https://github.com/apache/beam/pull/40740#issuecomment-6416610532
LGTM but I checked the actual failure logs and the root cause is in run Solace IO build script with `Unsupported class file major version 69 so the java-version default pin is enough to unblock so why add the test changes (ports / read poll / SEMP retries)? @schizophrenicmaniac
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org