Skip to content

Commit 62a4c6d

Browse files
committed
[aws2] Make SqsIOWriteBatchesTest timeout tests timing-independent
The four timeout-related tests asserted the exact grouping of messages into SendMessageBatch calls. Those groupings depend on wall-clock timing (the per-message Thread.sleep delay racing the configured batch timeout), so on loaded CI runners batches form differently and the strict verify(...).sendMessageBatch(request(exact entries)) checks fail with Mockito ArgumentsAreDifferent. Rewrite the assertions to verify timing-independent invariants instead: all expected message bodies are sent exactly once, no batch exceeds the size implied by the timeout cadence, and at least the minimum number of batches is produced. This still exercises the timeout-driven flushing (both synchronous and the strict separate-thread variant) without depending on exact wall-clock behavior. Fixes #38946
1 parent a88686c commit 62a4c6d

1 file changed

Lines changed: 75 additions & 22 deletions

File tree

sdks/java/io/amazon-web-services2/src/test/java/org/apache/beam/sdk/io/aws2/sqs/SqsIOWriteBatchesTest.java

Lines changed: 75 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -21,15 +21,18 @@
2121
import static java.nio.charset.StandardCharsets.UTF_8;
2222
import static java.util.concurrent.CompletableFuture.completedFuture;
2323
import static java.util.concurrent.CompletableFuture.supplyAsync;
24+
import static java.util.stream.Collectors.groupingBy;
2425
import static java.util.stream.Collectors.toList;
2526
import static java.util.stream.IntStream.range;
27+
import static org.apache.beam.sdk.util.Preconditions.checkStateNotNull;
2628
import static org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Preconditions.checkNotNull;
2729
import static org.assertj.core.api.Assertions.assertThat;
2830
import static org.assertj.core.api.Assertions.assertThatThrownBy;
2931
import static org.joda.time.Duration.millis;
3032
import static org.junit.Assert.assertEquals;
3133
import static org.junit.Assert.assertTrue;
3234
import static org.mockito.ArgumentMatchers.any;
35+
import static org.mockito.Mockito.atLeast;
3336
import static org.mockito.Mockito.atLeastOnce;
3437
import static org.mockito.Mockito.times;
3538
import static org.mockito.Mockito.verify;
@@ -38,6 +41,7 @@
3841

3942
import java.util.Arrays;
4043
import java.util.HashSet;
44+
import java.util.List;
4145
import java.util.Map;
4246
import java.util.Set;
4347
import java.util.stream.Collectors;
@@ -263,10 +267,16 @@ public void testWriteBatchesWithTimeout() {
263267

264268
p.run().waitUntilFinish();
265269

266-
SendMessageBatchRequestEntry[] entries = entries(range(0, 5));
267-
// due to added delay, batches are timed out on arrival of every 3rd msg
268-
verify(sqs).sendMessageBatch(request("queue", entries[0], entries[1], entries[2]));
269-
verify(sqs).sendMessageBatch(request("queue", entries[3], entries[4]));
270+
// Nominally batches time out on arrival of every 3rd message ([0,1,2], [3,4]). The exact
271+
// grouping depends on wall clock time and is unreliable on loaded machines (#38946), so
272+
// verify timing-independent invariants instead: expired batches are flushed on append, so
273+
// with >= 100ms between messages no batch can exceed 3 entries before exceeding the 150ms
274+
// timeout, forcing at least 2 batches for 5 messages.
275+
Map<String, List<SendMessageBatchRequest>> requests = captureBatchRequests(atLeast(2));
276+
assertThat(requests.keySet()).containsExactly("queue");
277+
assertMessageBodies(requestsFor(requests, "queue"), range(0, 5));
278+
assertThat(requestsFor(requests, "queue"))
279+
.allSatisfy(req -> assertThat(req.entries().size()).isLessThanOrEqualTo(3));
270280
}
271281

272282
@Test
@@ -285,11 +295,16 @@ public void testWriteBatchesWithStrictTimeout() {
285295

286296
p.run().waitUntilFinish();
287297

288-
SendMessageBatchRequestEntry[] entries = entries(range(0, 5));
289-
// using strict timeouts batches, batches are timed out by a separate thread
290-
verify(sqs).sendMessageBatch(request("queue", entries[0], entries[1]));
291-
verify(sqs).sendMessageBatch(request("queue", entries[2], entries[3]));
292-
verify(sqs).sendMessageBatch(request("queue", entries[4]));
298+
// Nominally the separate timeout thread flushes [0,1], [2,3], [4]. The exact grouping
299+
// depends on wall clock time and is unreliable on loaded machines (#38946), so verify
300+
// timing-independent invariants instead. Expired batches are also flushed on append
301+
// (independently of the timeout thread), so no batch can exceed 3 entries and 5 messages
302+
// require at least 2 batches.
303+
Map<String, List<SendMessageBatchRequest>> requests = captureBatchRequests(atLeast(2));
304+
assertThat(requests.keySet()).containsExactly("queue");
305+
assertMessageBodies(requestsFor(requests, "queue"), range(0, 5));
306+
assertThat(requestsFor(requests, "queue"))
307+
.allSatisfy(req -> assertThat(req.entries().size()).isLessThanOrEqualTo(3));
293308
}
294309

295310
@Test
@@ -337,11 +352,18 @@ public void testWriteBatchesToDynamicWithTimeout() {
337352

338353
p.run().waitUntilFinish();
339354

340-
SendMessageBatchRequestEntry[] entries = entries(range(0, 5));
341-
// due to added delay, dynamic batches are timed out on arrival of every 2nd msg (per batch)
342-
verify(sqs).sendMessageBatch(request("even", entries[0], entries[2]));
343-
verify(sqs).sendMessageBatch(request("uneven", entries[1], entries[3]));
344-
verify(sqs).sendMessageBatch(request("even", entries[4]));
355+
// Nominally dynamic batches are timed out on arrival of every 2nd message per batch
356+
// ([0,2], [1,3], [4]). The exact grouping depends on wall clock time and is unreliable on
357+
// loaded machines (#38946), so verify timing-independent invariants instead: per queue,
358+
// consecutive messages are >= 200ms apart, so expired batches are flushed on append once
359+
// reaching 2 entries, requiring at least 3 batches overall.
360+
Map<String, List<SendMessageBatchRequest>> requests = captureBatchRequests(atLeast(3));
361+
assertThat(requests.keySet()).containsExactlyInAnyOrder("even", "uneven");
362+
assertMessageBodies(requestsFor(requests, "even"), range(0, 5).filter(i -> i % 2 == 0));
363+
assertMessageBodies(requestsFor(requests, "uneven"), range(0, 5).filter(i -> i % 2 == 1));
364+
requests.values().stream()
365+
.flatMap(List::stream)
366+
.forEach(req -> assertThat(req.entries().size()).isLessThanOrEqualTo(2));
345367
}
346368

347369
@Test
@@ -360,14 +382,19 @@ public void testWriteBatchesToDynamicWithStrictTimeout() {
360382

361383
p.run().waitUntilFinish();
362384

363-
SendMessageBatchRequestEntry[] entries = entries(range(0, 5));
364-
// using strict timeouts batches, batches are timed out by a separate thread before any 2nd
365-
// entry
366-
verify(sqs).sendMessageBatch(request("even", entries[0]));
367-
verify(sqs).sendMessageBatch(request("uneven", entries[1]));
368-
verify(sqs).sendMessageBatch(request("even", entries[2]));
369-
verify(sqs).sendMessageBatch(request("uneven", entries[3]));
370-
verify(sqs).sendMessageBatch(request("even", entries[4]));
385+
// Nominally the separate timeout thread flushes every batch before a 2nd entry arrives
386+
// (5 singleton batches). The exact grouping depends on wall clock time and is unreliable
387+
// on loaded machines (#38946), so verify timing-independent invariants instead. Expired
388+
// batches are also flushed on append (independently of the timeout thread) and per queue
389+
// consecutive messages are >= 200ms apart, so no batch can exceed 2 entries and at least
390+
// 3 batches are required overall.
391+
Map<String, List<SendMessageBatchRequest>> requests = captureBatchRequests(atLeast(3));
392+
assertThat(requests.keySet()).containsExactlyInAnyOrder("even", "uneven");
393+
assertMessageBodies(requestsFor(requests, "even"), range(0, 5).filter(i -> i % 2 == 0));
394+
assertMessageBodies(requestsFor(requests, "uneven"), range(0, 5).filter(i -> i % 2 == 1));
395+
requests.values().stream()
396+
.flatMap(List::stream)
397+
.forEach(req -> assertThat(req.entries().size()).isLessThanOrEqualTo(2));
371398
}
372399

373400
@Test
@@ -406,6 +433,32 @@ private SendMessageBatchRequest anyRequest() {
406433
return any();
407434
}
408435

436+
/** Captures all batch requests, verifying the given mode, and groups them by queue url. */
437+
private Map<String, List<SendMessageBatchRequest>> captureBatchRequests(
438+
org.mockito.verification.VerificationMode mode) {
439+
ArgumentCaptor<SendMessageBatchRequest> captor =
440+
ArgumentCaptor.forClass(SendMessageBatchRequest.class);
441+
verify(sqs, mode).sendMessageBatch(captor.capture());
442+
return captor.getAllValues().stream().collect(groupingBy(SendMessageBatchRequest::queueUrl));
443+
}
444+
445+
/** Returns the (non-null) batch requests for a queue, failing if there were none. */
446+
private List<SendMessageBatchRequest> requestsFor(
447+
Map<String, List<SendMessageBatchRequest>> requests, String queue) {
448+
return checkStateNotNull(requests.get(queue), "no batch requests for queue %s", queue);
449+
}
450+
451+
/** Asserts that the requests contain exactly the expected message bodies, each exactly once. */
452+
private void assertMessageBodies(List<SendMessageBatchRequest> requests, IntStream expectedMsgs) {
453+
assertThat(
454+
requests.stream()
455+
.flatMap(req -> req.entries().stream())
456+
.map(SendMessageBatchRequestEntry::messageBody)
457+
.collect(toList()))
458+
.containsExactlyInAnyOrderElementsOf(
459+
expectedMsgs.mapToObj(Integer::toString).collect(toList()));
460+
}
461+
409462
private SendMessageBatchRequest request(String queue, SendMessageBatchRequestEntry... entries) {
410463
return SendMessageBatchRequest.builder()
411464
.queueUrl(queue)

0 commit comments

Comments
 (0)