Skip to content

Adding a cancelled field to tell if a cancellable task is cancelled - #1732

Merged
dblock merged 3 commits into
opensearch-project:mainfrom
meghasaik:main
Dec 16, 2021
Merged

Adding a cancelled field to tell if a cancellable task is cancelled#1732
dblock merged 3 commits into
opensearch-project:mainfrom
meghasaik:main

Conversation

@meghasaik

Copy link
Copy Markdown
Contributor

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
New functionality includes testing.
All tests pass
New functionality has been documented.
New functionality has javadoc added
Commits are signed per the DCO using --signoff
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.

@meghasaik
meghasaik requested a review from a team as a code owner December 15, 2021 03:28
@meghasaik meghasaik changed the title Changes made by adding the Task indicator and the IllegalArgs. Adding a cancelled field to tell if a cancellable task is cancelled Dec 15, 2021
@opensearch-ci-bot

Copy link
Copy Markdown
Contributor

Can one of the admins verify this patch?

Signed-off-by: meghasaik <kavmegha@amazon.com>
@opensearch-ci-bot

Copy link
Copy Markdown
Contributor

❌   Gradle Check failure e30f75f5affbaea1b0e497287fc4c9a555eed435
Log 1500

Reports 1500

@opensearch-ci-bot

Copy link
Copy Markdown
Contributor

❌   Gradle Check failure db7eda5
Log 1501

Reports 1501

@dblock dblock left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Look through failing gradle checks.

Would change the message, otherwise LGTM. It's still wrong that TaskInfo works for multiple kinds of tasks and that Task knows about its child. I would refactor that implementation, but that can be a different PR.

Map<String, String> headers
) {
if (cancellable == false && cancelled == true) {
throw new IllegalArgumentException("uncancellable task cannot be cancelled");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it should just say "task cannot be cancelled", it implies the former

cancelled = false;
}
if (cancellable == false && cancelled == true) {
throw new IllegalArgumentException("uncancellable task cannot be cancelled");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

task cannot be cancelled

@andrross

Copy link
Copy Markdown
Member
Execution failed for task ':distribution:bwc:bugfix:buildBwcLinuxTar'.
> Building 1.2.1 didn't generate expected file /var/CITOOL/workflow/OpenSearch_CI/PR_Checks/Gradle_Check/search/distribution/bwc/bugfix/build/bwc/checkout-1.2/distribution/ar
chives/linux-tar/build/distributions/opensearch-min-1.2.1-SNAPSHOT-linux-x64.tar.gz

I think you need to rebase another version update commit.

@opensearch-ci-bot

Copy link
Copy Markdown
Contributor

✅   Gradle Check success 8856a74
Log 1510

Reports 1510

Signed-off-by: meghasaik <kavmegha@amazon.com>
@opensearch-ci-bot

Copy link
Copy Markdown
Contributor

✅   Gradle Check success 2a34fd1
Log 1515

Reports 1515

@dblock
dblock merged commit 22bfadf into opensearch-project:main Dec 16, 2021
@dblock

dblock commented Nov 28, 2022

Copy link
Copy Markdown
Member

@meghasaik FYI, there was a regression in this PR, see #5377

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants