-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
[Profiling] Use CancellableTask internally #107139
[Profiling] Use CancellableTask internally #107139
Conversation
With this commit we eagerly cast the task provided to our central transport action to a CancellableTask so we can simplify cancellation checks while the action is being executed. Relates elastic#107037
This PR is a follow-up based on a comment from David in #107037 (comment). |
Pinging @elastic/obs-knowledge-team (Team:obs-knowledge) |
licenseChecker.requireSupportedLicense(); | ||
assert task instanceof CancellableTask; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using asserts here might look unusual but that is the established practice in the code base.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ it indicates to the reader that task
is always a CancellableTask
- without it a reader might think that the cast could throw an exception which is to be handled elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
licenseChecker.requireSupportedLicense(); | ||
assert task instanceof CancellableTask; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ it indicates to the reader that task
is always a CancellableTask
- without it a reader might think that the cast could throw an exception which is to be handled elsewhere.
Thanks for quick review @DaveCTurner, much appreciated. |
With this commit we eagerly cast the task provided to our central transport action to a CancellableTask so we can simplify cancellation checks while the action is being executed.
Relates #107037