Adding a cancelled field to tell if a cancellable task is cancelled - #1682
Adding a cancelled field to tell if a cancellable task is cancelled #1682meghasaik wants to merge 4 commits into
Conversation
Signed-off-by: Megha Sai Kavikondala <kavmegha@amazon.com>
|
Can one of the admins verify this patch? |
| */ | ||
| public final class TaskInfo implements Writeable, ToXContentFragment { | ||
|
|
||
| static final String INCLUDE_CANCELLED_PARAM = "include_cancelled"; |
There was a problem hiding this comment.
I am not super familiar with this code, why is this necessary? Do we do something similar elsewhere?
There was a problem hiding this comment.
We don’t want this field to be shown if task is completed and also as an entry in the task index as we are not mapping this dynamically.
There was a problem hiding this comment.
Where else are we doing something similar?
There was a problem hiding this comment.
I also don't see any new tests that utilize this behavior.
There was a problem hiding this comment.
Why don't we want this field to be shown if the task is completed?
If being completed it intended to be mutually exclusive from being cancelled, did you consider adding something like a state field where the value could be one of RUNNING, CANCELLED or COMPLETED?
There was a problem hiding this comment.
As the cancelled field tells about the task being cancelled or not, having it when the task is completed felt unnecessary. If the task is completed it means that it has not been cancelled and as mentioned in the issue, if the task is cancelled it will eventually abort/fail. I think the cancelled field does the job of the state field that you mentioned as it shows the false = task is running and true = task is cancelled.
There was a problem hiding this comment.
it shows the false = task is running and true = task is cancelled
The logic here implicitly has a third state of the cancelled boolean, which is "not present = task is complete". If there is a strong reason to have that third state then using a type that can explicitly represent three states is a better way to go. However, I'd consider doing the simpler approach of always including the cancelled boolean, even on complete tasks when it will be set to false.
There was a problem hiding this comment.
I have added the cancelled field to be shown as a field when the task is completed.
| assertEquals(((Number) map.get("start_time_in_millis")).longValue(), startTime); | ||
| assertEquals(((Number) map.get("running_time_in_nanos")).longValue(), runningTime); | ||
| assertEquals(map.get("cancellable"), cancellable); | ||
| if (cancellable) { |
There was a problem hiding this comment.
Having if inside tests is an antipattern, it causes tests to start randomly failing. This test will randomly flip cancellable and not and sometimes test the functionality, and sometimes not. Split into three tests: the existing one that tests the kitchen sink, one that only tests cancellable and the other than only tests non-cancellable options.
There was a problem hiding this comment.
Sure, I will divide the test into three other tests for more clarity.
| startTime, | ||
| System.nanoTime() - startTimeNanos, | ||
| this instanceof CancellableTask, | ||
| this instanceof CancellableTask && ((CancellableTask) this).isCancelled(), |
There was a problem hiding this comment.
I don't know to be honest why this method is final, but it certainly looks like it should not be and should be overridden by CancellableTask, without the need to use instanceof and type casting.
There was a problem hiding this comment.
Agreed. It seems to defeat the purpose of inheritance to have a parent class check at runtime whether it is an instance of a child class.
Signed-off-by: Megha Sai Kavikondala <kavmegha@amazon.com>
…asaik/OpenSearch into task-cancellation-indicator Signed-off-by: Megha Sai Kavikondala <kavmegha@amazon.com>
| <<<<<<< HEAD | ||
| ======= |
| } else { | ||
| cancelled = false; | ||
| } | ||
| assert cancellable || cancelled == false : "uncancellable task cannot be cancelled"; |
There was a problem hiding this comment.
I'm not sure assert is the right thing to do here. An AssertionError (because it is a child of error) "indicates serious problems that a reasonable application should not try to catch". This is constructing an object from parsing a stream, so are there contexts where external input could trigger this path? If so, we definitely don't want the application to throw an Error. IllegalArgumentException seems more appropriate to me.
There was a problem hiding this comment.
This might trigger in those cases where we are trying to cancel a task which cannot be cancelled when cancelled is true and cancellable is false. But as you mentioned we can throw a IllegalArgumentException instead of the assert error.
|
Closing this PR out as I was getting merge conflicts. |
5 participants
Signed-off-by: Megha Sai Kavikondala kavmegha@amazon.com
Description
A cancelled field is added which tells if the cancellable task is cancelled. At some points, even if the cancellable task is cancelled it still runs, so to show that the task is cancelled, a cancelled field is shown in the tasks API,
cancelled:true.Issues Resolved
#520
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.