Skip to content

Remove needless use of assertBusy polling Future.isDone #119078

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 19, 2024
Merged

Remove needless use of assertBusy polling Future.isDone #119078

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion modules/reindex/src/test/java/org/elasticsearch/reindex/AsyncBulkByScrollActionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,6 @@ public void testStartRetriesOnRejectionButFailsOnTooManyRejections() throws Exce
DummyAsyncBulkByScrollAction action = new DummyActionWithoutBackoff();
action.start();
assertBusy(() -> assertEquals(testRequest.getMaxRetries() + 1, client.searchAttempts.get()));
assertBusy(() -> assertTrue(listener.isDone()));
ExecutionException e = expectThrows(ExecutionException.class, () -> listener.get());
assertThat(ExceptionsHelper.stackTrace(e), containsString(EsRejectedExecutionException.class.getSimpleName()));
assertNull("There shouldn't be a search attempt pending that we didn't reject", client.lastSearch.get());
Expand Down
6 changes: 4 additions & 2 deletions ...lusterTest/java/org/elasticsearch/action/admin/cluster/node/tasks/CancellableTasksIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,11 +252,13 @@ public void testDoNotWaitForCompletion() throws Exception {
if (waitForCompletion) {
assertFalse(cancelFuture.isDone());
} else {
assertBusy(() -> assertTrue(cancelFuture.isDone()));
cancelFuture.get();
}
allowEntireRequest(rootRequest);
waitForRootTask(mainTaskFuture, false);
cancelFuture.actionGet();
if (waitForCompletion) {
cancelFuture.actionGet();
}
ensureBansAndCancellationsConsistency();
}

Expand Down
23 changes: 14 additions & 9 deletions server/src/internalClusterTest/java/org/elasticsearch/search/ccs/CrossClusterIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,11 @@ public void testProxyConnectionDisconnect() throws Exception {
}
}
});
assertBusy(() -> assertTrue(future.isDone()));
try {
future.get();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you use a timed get to avoid an infinite wait scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question :) "infinite wait" practically means 20min with the suite timeout we have in place.
To be honest, I kinda like not putting timeouts on these things personally. If the listener leaks due to a bug, yes CI will run for 20 min, but so be it? :) If I fail after a 20 min wait, I'm close to 100% sure it wasn't a slow CI run that failed things. Otoh, if it's a 10s wait ... the first thing my mind will jump to is wonder if we conceivably could have had have a 10s GC or so pause here :)
-> personally I wouldn't add timeouts on future.get() in tests anywhere pretty much I think.

} catch (ExecutionException e) {
// ignored
}
configureAndConnectsToRemoteClusters();
} finally {
SearchListenerPlugin.allowQueryPhase();
Expand Down Expand Up @@ -298,20 +302,21 @@ public void testCancel() throws Exception {
}

SearchListenerPlugin.allowQueryPhase();
assertBusy(() -> assertTrue(queryFuture.isDone()));
assertBusy(() -> assertTrue(cancelFuture.isDone()));
try {
queryFuture.get();
fail("query should have failed");
} catch (ExecutionException e) {
assertNotNull(e.getCause());
Throwable t = ExceptionsHelper.unwrap(e, TaskCancelledException.class);
assertNotNull(t);
}
cancelFuture.get();
assertBusy(() -> {
final Iterable<TransportService> transportServices = cluster("cluster_a").getInstances(TransportService.class);
for (TransportService transportService : transportServices) {
assertThat(transportService.getTaskManager().getBannedTaskIds(), Matchers.empty());
}
});

ExecutionException e = expectThrows(ExecutionException.class, () -> queryFuture.result());
assertNotNull(e);
assertNotNull(e.getCause());
Throwable t = ExceptionsHelper.unwrap(e, TaskCancelledException.class);
assertNotNull(t);
}

/**
Expand Down
8 changes: 2 additions & 6 deletions server/src/internalClusterTest/java/org/elasticsearch/search/ccs/CrossClusterSearchIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,11 +379,9 @@ public void testClusterDetailsAfterCCSWithFailuresOnRemoteClusterOnly() throws E
r.incRef();
l.onResponse(r);
}));
assertBusy(() -> assertTrue(queryFuture.isDone()));

// dfs=true overrides the minimize_roundtrips=true setting and does not minimize roundtrips
if (skipUnavailable == false && minimizeRoundtrips && dfs == false) {
ExecutionException ee = expectThrows(ExecutionException.class, () -> queryFuture.get());
ExecutionException ee = expectThrows(ExecutionException.class, queryFuture::get);
assertNotNull(ee.getCause());
assertThat(ee.getCause(), instanceOf(RemoteTransportException.class));
Throwable rootCause = ExceptionsHelper.unwrap(ee.getCause(), IllegalStateException.class);
Expand Down Expand Up @@ -622,10 +620,8 @@ public void testRemoteClusterOnlyCCSWithFailuresOnAllShards() throws Exception {
r.incRef();
l.onResponse(r);
}));
assertBusy(() -> assertTrue(queryFuture.isDone()));

if (skipUnavailable == false || minimizeRoundtrips == false) {
ExecutionException ee = expectThrows(ExecutionException.class, () -> queryFuture.get());
ExecutionException ee = expectThrows(ExecutionException.class, queryFuture::get);
assertNotNull(ee.getCause());
Throwable rootCause = ExceptionsHelper.unwrap(ee, IllegalStateException.class);
assertThat(rootCause.getMessage(), containsString("index corrupted"));
Expand Down
9 changes: 6 additions & 3 deletions server/src/internalClusterTest/java/org/elasticsearch/snapshots/ConcurrentSnapshotsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1361,9 +1361,12 @@ public void testConcurrentOperationsLimit() throws Exception {
if (deleteAndAbortAll) {
awaitNumberOfSnapshotsInProgress(0);
for (ActionFuture<CreateSnapshotResponse> snapshotFuture : snapshotFutures) {
// just check that the futures resolve, whether or not things worked out with the snapshot actually finalizing or failing
// due to the abort does not matter
assertBusy(() -> assertTrue(snapshotFuture.isDone()));
try {
snapshotFuture.get();
} catch (ExecutionException e) {
// just check that the futures resolve, whether or not things worked out with the snapshot actually finalizing or
// failing due to the abort does not matter
}
}
assertThat(getRepositoryData(repoName).getSnapshotIds(), empty());
} else {
Expand Down
1 change: 0 additions & 1 deletion ...asticsearch/action/admin/indices/diskusage/TransportAnalyzeIndexDiskUsageActionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,6 @@ public void testSimpleLimitRequests() throws Exception {
assertBusy(() -> assertThat(transportService.getRequestsSentPerNode(), equalTo(expectedRequestCounts)));
pendingRequests.addAll(transportService.getCapturedRequests(true));
}
assertBusy(future::isDone);
AnalyzeIndexDiskUsageResponse response = future.actionGet();
assertThat(response.getTotalShards(), equalTo(numberOfShards));
assertThat(response.getFailedShards(), equalTo(0));
Expand Down
3 changes: 1 addition & 2 deletions ...k/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plugin/ComputeListenerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,8 +460,7 @@ public void testCancelOnFailure() throws Exception {
}), TimeValue.timeValueNanos(between(0, 100)), threadPool.generic());
}
}
assertBusy(rootListener::isDone);
ExecutionException failure = expectThrows(ExecutionException.class, () -> rootListener.get(1, TimeUnit.SECONDS));
ExecutionException failure = expectThrows(ExecutionException.class, () -> rootListener.get(10, TimeUnit.SECONDS));
Throwable cause = failure.getCause();
assertNotNull(failure);
assertThat(cause, instanceOf(CircuitBreakingException.class));
Expand Down