Skip to content
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.

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

Do not throw in task enqueued by CancellableRunner #112780

Merged
merged 3 commits into from
Sep 12, 2024

Conversation

ywangd
Copy link
Member

@ywangd ywangd commented Sep 12, 2024

CancellableThreads#excute can throw runtime exception including cancellation. This does not work with AbstractThrottledTaskRunner which expects enqueued task to not throw. This PR catches any runtime exception from CancellableThreads and hand it back to the original runnable.

Resolves: #112779

CancellableThreads#eecute can throw runtime exception including
cancellation. This does not work with AbstractThrottledTaskRunner which
expects enqueued task to _not_ throw. This PR catches any runtime
exception from CancellableThreads and hand it back to the original
runnable.

Resolves: elastic#112779
@ywangd ywangd added >non-issue :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.16.0 labels Sep 12, 2024
@elasticsearchmachine elasticsearchmachine added Team:Distributed Meta label for distributed team (obsolete) v9.0.0 labels Sep 12, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM thanks Yang. I wonder if we should make it so that CancellableThreads#execute doesn't throw anything itself, or introduce a variant which integrates better with AbstractRunnable, but that's a question for a later day.

@ywangd ywangd added auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) auto-backport-and-merge labels Sep 12, 2024
@ywangd
Copy link
Member Author

ywangd commented Sep 12, 2024

@elasticmachine update branch

Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

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

LGTM, nice find!

@elasticsearchmachine elasticsearchmachine merged commit e22bef6 into elastic:main Sep 12, 2024
15 checks passed
@ywangd ywangd deleted the es-112779-fix branch September 12, 2024 06:13
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.x Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 112780

ywangd added a commit to ywangd/elasticsearch that referenced this pull request Sep 12, 2024
CancellableThreads#excute can throw runtime exception including
cancellation. This does not work with AbstractThrottledTaskRunner which
expects enqueued task to _not_ throw. This PR catches any runtime
exception from CancellableThreads and hand it back to the original
runnable.

Resolves: elastic#112779
(cherry picked from commit e22bef6)

# Conflicts:
#	muted-tests.yml
@ywangd
Copy link
Member Author

ywangd commented Sep 12, 2024

💚 All backports created successfully

Status Branch Result
8.x

Questions ?

Please refer to the Backport tool documentation

v1v added a commit to v1v/elasticsearch that referenced this pull request Sep 12, 2024
…tion-ironbank-ubi

* upstream/main: (302 commits)
  Deduplicate BucketOrder when deserializing (elastic#112707)
  Introduce test utils for ingest pipelines (elastic#112733)
  [Test] Account for auto-repairing for shard gen file (elastic#112778)
  Do not throw in task enqueued by CancellableRunner (elastic#112780)
  Mute org.elasticsearch.script.StatsSummaryTests testEqualsAndHashCode elastic#112439
  Mute org.elasticsearch.repositories.blobstore.testkit.integrity.RepositoryVerifyIntegrityIT testTransportException elastic#112779
  Use a dedicated test executor in MockTransportService (elastic#112748)
  Estimate segment field usages (elastic#112760)
  (Doc+) Inference Pipeline ignores Mapping Analyzers (elastic#112522)
  Fix verifyVersions task (elastic#112765)
  (Doc+) Terminating Exit Codes (elastic#112530)
  (Doc+) CAT Nodes default columns (elastic#112715)
  [DOCS] Augment installation warnings (elastic#112756)
  Mute org.elasticsearch.repositories.blobstore.testkit.integrity.RepositoryVerifyIntegrityIT testCorruption elastic#112769
  Bump Elasticsearch to a minimum of JDK 21 (elastic#112252)
  ESQL: Compute support for filtering ungrouped aggs (elastic#112717)
  Bump Elasticsearch version to 9.0.0 (elastic#112570)
  add CDR related data streams to kibana_system priviliges (elastic#112655)
  Support widening of numeric types in union-types (elastic#112610)
  Introduce data stream options and failure store configuration classes (elastic#109515)
  ...
elasticsearchmachine pushed a commit that referenced this pull request Sep 12, 2024
…112786)

* Do not throw in task enqueued by CancellableRunner (#112780)

CancellableThreads#excute can throw runtime exception including
cancellation. This does not work with AbstractThrottledTaskRunner which
expects enqueued task to _not_ throw. This PR catches any runtime
exception from CancellableThreads and hand it back to the original
runnable.

Resolves: #112779
(cherry picked from commit e22bef6)

# Conflicts:
#	muted-tests.yml

* Update muted-tests.yml

* Update muted-tests.yml

---------

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
davidkyle pushed a commit that referenced this pull request Sep 12, 2024
CancellableThreads#excute can throw runtime exception including
cancellation. This does not work with AbstractThrottledTaskRunner which
expects enqueued task to _not_ throw. This PR catches any runtime
exception from CancellableThreads and hand it back to the original
runnable.

Resolves: #112779
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) backport pending :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >non-issue Team:Distributed Meta label for distributed team (obsolete) v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] RepositoryVerifyIntegrityIT testTransportException failing
5 participants