-
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
[Reporting/CSV] Do not fail the job if scroll ID can not be cleared #76014
Conversation
Pinging @elastic/kibana-reporting-services (Team:Reporting Services) |
@elasticmachine merge upstream |
@elasticmachine Merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
return callEndpoint('clearScroll', { | ||
scrollId: [scrollId], | ||
}); | ||
try { |
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.
Should we wrap these in a try/catch, or do:
return callEndpoint('clearScroll', { scrollId: [scrollId] }).catch((err) => {
// Do not throw the error, as the job can still be completed successfully
logger.warn('Scroll context can not be cleared!');
logger.error(err);
});
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.
Just a matter of code/preference. The .catch
is a bit more concise whereas the try/catch is more idiomatic async/await js
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 definitely could go either way, in which case I like to defer to the existing async style in other code in the file. In this case, async/await follows the code style in this file
Thank @joelgriffith ! |
…lastic#76014) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* master: (223 commits) skip flaky suite (elastic#75724) [Reporting] Add functional test for Reports in non-default spaces (elastic#76053) [Enterprise Search] Fix various icons in dark mode (elastic#76430) skip flaky suite (elastic#76245) Add `auto` interval to histogram AggConfig (elastic#76001) [Resolver] generator uses setup_node_env (elastic#76422) [Ingest Manager] Support both zip & tar archives from Registry (elastic#76197) [Ingest Manager] Improve agent vs kibana version checks (elastic#76238) Manually building `KueryNode` for Fleet's routes (elastic#75693) remove dupe tinymath section (elastic#76093) Create APM issue template (elastic#76362) Delete unused file. (elastic#76386) [SECURITY_SOLUTION][ENDPOINT] Trusted Apps Create API (elastic#76178) [Detections Engine] Add Alert actions to the Timeline (elastic#73228) [Dashboard First] Library Notification (elastic#76122) [Maps] Add mvt support for ES doc sources (elastic#75698) Add setHeaderActionMenu API to AppMountParameters (elastic#75422) [ML] Remove "Are you sure" from data frame analytics jobs (elastic#76214) [yarn] remove typings-tester, use @ts-expect-error (elastic#76341) [Reporting/CSV] Do not fail the job if scroll ID can not be cleared (elastic#76014) ...
Summary
Closes #75594
Release note:
This PR avoids failing the CSV Reporting job if the call to clear the scroll fails. At that point, the CSV content has already been generated and is ready for export.
Reasons why Elasticsearch returns a 404 when called to clear a scroll:
If Elasticsearch can not find an active search context, it will return a 404 Not Found HTTP status code.
Checklist
Delete any items that are not applicable to this PR.
For maintainers