-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[chore] Improve request cancelation handling in vis embeddable #65057
Conversation
Pinging @elastic/kibana-app-arch (Team:AppArch) |
💚 Build SucceededTo update your PR or re-run it, just comment with: |
@@ -331,13 +331,14 @@ export class VisualizeEmbeddable extends Embeddable<VisualizeInput, VisualizeOut | |||
this.abortController.abort(); | |||
} | |||
this.abortController = new AbortController(); | |||
const abortController = this.abortController; |
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.
why creating local variable ? if we have it lets use it everywhere, but without it this can be a one line PR.
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.
local variable here is needed as a reference to previous abortController
instance.
when await buildPipeline
is finished, in case it was aborted, this.abortController
would be a reference from next request
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.
code LGTM
* master: (72 commits) add tsvb tests to Firefox suite (elastic#65425) Fix flaky ServerMetricsCollector integration test (elastic#65420) [APM] Custom links section inside the Actions menu is showing outside of the menu (elastic#65428) [ML] Adds docs_per_second to transform edit form. (elastic#65365) update apm index pattern (elastic#65424) add direct build command (elastic#65431) [ML] Adding daily_model_snapshot_retention_after_days to types and schemas (elastic#65417) [chore] Improve request cancelation handling in vis embeddable (elastic#65057) [Alerting] migrates acceptance and functional test fixtures to KP (elastic#64888) [ML] Fixes reordering in view by selection when overall cell selected (elastic#65290) Additional branding updates (elastic#64712) Remove redundant formatting of percentage column (elastic#64948) [SIEM][CASE] Configuration pages UI redesign (elastic#65355) New nav (elastic#64018) [Ingest pipelines] Address copy feedback (elastic#65175) bug fixing (elastic#65387) skip whole suite blocking snapshots (elastic#65377) add related event generation to ancestor nodes (fixes a bug) (elastic#64950) [Canvas] move files from legacy/plugins to plugins (elastic#65283) [SIEM] template timeline UI (elastic#64439) ...
Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync. |
… (#65636) Co-authored-by: Elastic Machine <[email protected]>
Summary
Follow up on: #61279
This fixes an edge case of cancelation handling if cancelation is happed inside
buildPipeline
function. e.g. when calculating interval for a histogram.For example,
currently, when calculating the interval for histogram and cancelation has happened, the error is swallowed, and we continue with expression execution for that request. Even though its result is no longer needed and we can reduce the amount of work and save some bandwidth.
(sorry, skipping test, as no existing suite for
visualise_embeddable
and no way to nicely cover it in functional suite)Checklist
Delete any items that are not applicable to this PR.
For maintainers