Skip to content

Commit 15fef0d

Browse files
mhl-balbertzaharovits
authored andcommitted
Use opaque id in task cancellation assertion (#110680)
Add use of Opaque ID HTTP header in task cancellation assertion. In some tests, like this #88201 `testCatSegmentsRestCancellation`, we assert that all tasks related to specific HTTP request are cancelled. But we do blanket approach in assertion block catching all tasks by action name. I think narrowing down assertion to specific http request in this case would be more accurate. It is still not clear why test mentioned above failing, but after hours of investigation and injecting random delays, I'm inclining more to @DaveCTurner's comment about interference from other tests or cluster activity. I added additional log that will report when we spot task with different opaque id.
1 parent 6807bd5 commit 15fef0d

File tree

4 files changed

+32
-18
lines changed

4 files changed

+32
-18
lines changed

qa/smoke-test-http/src/javaRestTest/java/org/elasticsearch/http/BlockedSearcherRestCancellationTestCase.java

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
import org.elasticsearch.indices.IndicesService;
3131
import org.elasticsearch.plugins.EnginePlugin;
3232
import org.elasticsearch.plugins.Plugin;
33+
import org.elasticsearch.tasks.Task;
3334

3435
import java.util.ArrayList;
3536
import java.util.Collection;
@@ -76,6 +77,10 @@ void runTest(Request request, String actionPrefix) throws Exception {
7677
createIndex("test", Settings.builder().put(BLOCK_SEARCHER_SETTING.getKey(), true).build());
7778
ensureGreen("test");
7879

80+
assert request.getOptions().containsHeader(Task.X_OPAQUE_ID_HTTP_HEADER) == false;
81+
final var opaqueId = getTestClass().getSimpleName() + "-" + getTestName() + "-" + randomUUID();
82+
request.setOptions(request.getOptions().toBuilder().addHeader(Task.X_OPAQUE_ID_HTTP_HEADER, opaqueId));
83+
7984
final List<Semaphore> searcherBlocks = new ArrayList<>();
8085
for (final IndicesService indicesService : internalCluster().getInstances(IndicesService.class)) {
8186
for (final IndexService indexService : indicesService) {
@@ -96,7 +101,8 @@ void runTest(Request request, String actionPrefix) throws Exception {
96101
}
97102

98103
final PlainActionFuture<Response> future = new PlainActionFuture<>();
99-
logger.info("--> sending request");
104+
logger.info("--> sending request, opaque id={}", opaqueId);
105+
100106
final Cancellable cancellable = getRestClient().performRequestAsync(request, wrapAsRestResponseListener(future));
101107

102108
awaitTaskWithPrefix(actionPrefix);
@@ -108,7 +114,7 @@ void runTest(Request request, String actionPrefix) throws Exception {
108114
cancellable.cancel();
109115
expectThrows(CancellationException.class, future::actionGet);
110116

111-
assertAllCancellableTasksAreCancelled(actionPrefix);
117+
assertAllCancellableTasksAreCancelled(actionPrefix, opaqueId);
112118
} finally {
113119
Releasables.close(releasables);
114120
}

server/src/main/java/org/elasticsearch/tasks/CancellableTask.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,11 @@ public final <T> boolean notifyIfCancelled(ActionListener<T> listener) {
109109
return true;
110110
}
111111

112+
@Override
113+
public String toString() {
114+
return "CancellableTask{" + super.toString() + ", reason='" + reason + '\'' + ", isCancelled=" + isCancelled + '}';
115+
}
116+
112117
private TaskCancelledException getTaskCancelledException() {
113118
assert Thread.holdsLock(this);
114119
assert isCancelled;

server/src/main/java/org/elasticsearch/tasks/Task.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,8 @@ public String toString() {
225225
+ parentTask
226226
+ ", startTime="
227227
+ startTime
228+
+ ", headers="
229+
+ headers
228230
+ ", startTimeNanos="
229231
+ startTimeNanos
230232
+ '}';

test/framework/src/main/java/org/elasticsearch/test/TaskAssertions.java

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -10,16 +10,19 @@
1010

1111
import org.apache.logging.log4j.LogManager;
1212
import org.apache.logging.log4j.Logger;
13+
import org.elasticsearch.core.Nullable;
1314
import org.elasticsearch.tasks.CancellableTask;
1415
import org.elasticsearch.tasks.Task;
1516
import org.elasticsearch.tasks.TaskInfo;
16-
import org.elasticsearch.tasks.TaskManager;
1717
import org.elasticsearch.transport.TransportService;
1818

19+
import java.util.ArrayList;
1920
import java.util.List;
21+
import java.util.Objects;
2022
import java.util.concurrent.TimeUnit;
2123
import java.util.stream.Collectors;
2224

25+
import static junit.framework.TestCase.assertFalse;
2326
import static junit.framework.TestCase.assertTrue;
2427
import static junit.framework.TestCase.fail;
2528
import static org.elasticsearch.test.ESIntegTestCase.client;
@@ -59,30 +62,28 @@ private static void awaitTaskWithPrefix(String actionPrefix, Iterable<TransportS
5962
});
6063
}
6164

62-
public static void assertAllCancellableTasksAreCancelled(String actionPrefix) throws Exception {
65+
public static void assertAllCancellableTasksAreCancelled(String actionPrefix, @Nullable String opaqueId) throws Exception {
6366
logger.info("--> checking that all tasks with prefix {} are marked as cancelled", actionPrefix);
6467

6568
assertBusy(() -> {
66-
boolean foundTask = false;
69+
var tasks = new ArrayList<CancellableTask>();
6770
for (TransportService transportService : internalCluster().getInstances(TransportService.class)) {
68-
final TaskManager taskManager = transportService.getTaskManager();
71+
var taskManager = transportService.getTaskManager();
6972
assertTrue(taskManager.assertCancellableTaskConsistency());
70-
for (CancellableTask cancellableTask : taskManager.getCancellableTasks().values()) {
71-
if (cancellableTask.getAction().startsWith(actionPrefix)) {
72-
logger.trace("--> found task with prefix [{}]: [{}]", actionPrefix, cancellableTask);
73-
foundTask = true;
74-
assertTrue(
75-
"task " + cancellableTask.getId() + "/" + cancellableTask.getAction() + " not cancelled",
76-
cancellableTask.isCancelled()
77-
);
78-
logger.trace("--> Task with prefix [{}] is marked as cancelled: [{}]", actionPrefix, cancellableTask);
79-
}
80-
}
73+
taskManager.getCancellableTasks().values().stream().filter(t -> t.getAction().startsWith(actionPrefix)).forEach(tasks::add);
8174
}
82-
assertTrue("found no cancellable tasks", foundTask);
75+
assertFalse("no tasks found for action: " + actionPrefix, tasks.isEmpty());
76+
assertTrue(
77+
tasks.toString(),
78+
tasks.stream().allMatch(t -> t.isCancelled() && Objects.equals(t.getHeader(Task.X_OPAQUE_ID_HTTP_HEADER), opaqueId))
79+
);
8380
}, 30, TimeUnit.SECONDS);
8481
}
8582

83+
public static void assertAllCancellableTasksAreCancelled(String actionPrefix) throws Exception {
84+
assertAllCancellableTasksAreCancelled(actionPrefix, null);
85+
}
86+
8687
public static void assertAllTasksHaveFinished(String actionPrefix) throws Exception {
8788
logger.info("--> checking that all tasks with prefix {} have finished", actionPrefix);
8889
assertBusy(() -> {

0 commit comments

Comments
 (0)