Use opaque id in task cancellation assertion #110680
Conversation
mhl-b
added
>test
|
Pinging @elastic/es-distributed (Team:Distributed) |
| @@ -70,6 +72,14 @@ public static void assertAllCancellableTasksAreCancelled(String actionPrefix) th | |||
| for (CancellableTask cancellableTask : taskManager.getCancellableTasks().values()) { | |||
| if (cancellableTask.getAction().startsWith(actionPrefix)) { | |||
| logger.trace("--> found task with prefix [{}]: [{}]", actionPrefix, cancellableTask); | |||
| if (taskOpaqueIdMatches(cancellableTask.headers(), opaqueId) == false) { | |||
There was a problem hiding this comment.
I would rather not leniently skip non-matching tasks: we shouldn't have any such tasks in the first place. If this is the problem then I'd prefer to track down where those tasks are coming from and get rid of them at source.
Let's assert that the task ID matches here instead.
| // returns new or existing id | ||
| private static String setOpaqueIdIfNotExists(Request request) { | ||
| String opaqueId; | ||
| if (request.getOptions().containsHeader(Task.X_OPAQUE_ID_HTTP_HEADER)) { |
There was a problem hiding this comment.
I think this is dead code today since none of the requests include an opaque-id header. Let's just assert it's absent.
| } | ||
|
|
||
| private static boolean taskOpaqueIdMatches(Map<String, String> taskHeaders, @Nullable String opaqueId) { | ||
| if (opaqueId == null) { |
There was a problem hiding this comment.
I think we should assert the opaque ID is missing if we didn't expect to see one.
|
@DaveCTurner thank you for feedback. I added assertions and test names to the opaque id. Also added more explicit exception message in |
| @@ -71,11 +72,16 @@ protected boolean addMockInternalEngine() { | |||
| return false; | |||
| } | |||
|
|
|||
| void runTest(Request request, String actionPrefix) throws Exception { | |||
| void runTest(String testName, Request request, String actionPrefix) throws Exception { | |||
There was a problem hiding this comment.
Rather than passing in a string that (hopefully) identifies the test, I'd suggest using org.apache.lucene.tests.util.LuceneTestCase#getTestName (and maybe org.apache.lucene.tests.util.LuceneTestCase#getTestClass).
| @@ -19,9 +19,9 @@ | |||
| */ | |||
| public class CancellableTask extends Task { | |||
|
|
|||
| private final SubscribableListener<Void> listeners = new SubscribableListener<>(); | |||
There was a problem hiding this comment.
Nit: did you mean to move this field?
There was a problem hiding this comment.
applied formatting to entire file...
| public static void assertAllTasksHaveFinished(String actionPrefix) throws Exception { | ||
| logger.info("--> checking that all tasks with prefix {} have finished", actionPrefix); | ||
| assertBusy(() -> { | ||
| final List<TaskInfo> tasks = client().admin().cluster().prepareListTasks().get().getTasks(); | ||
| assertTrue(tasks.toString(), tasks.stream().noneMatch(t -> t.action().startsWith(actionPrefix))); | ||
| }); | ||
| } | ||
|
|
mhl-b
added
the
auto-merge-without-approval
mhl-b
changed the title
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.
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.
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.