-
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
removes usage of the _id field in Task manager #54765
removes usage of the _id field in Task manager #54765
Conversation
* upstream/master: (26 commits) Take page offset into account too (elastic#54567) [APM] Support error.{log,exception}.stacktrace.classname (elastic#54577) Np migration tsvb route validation (elastic#51850) [ML] MML calculator enhancements for multi-metric job wizard (elastic#54573) [SIEM] Fix Inspect query 'request timestamp' value changes when curso… (elastic#54223) Fix chromeless NP apps not using full page width (elastic#54550) Remove extraneous public import to prevent failing Kibana startup (elastic#54676) [Uptime] Temporarily skip flakey tests (elastic#54675) Skip failing uptime tests Create UI for alerting and actions plugin (elastic#48959) [dev/build/sass] build stylesheets for disabled plugins too (elastic#54654) [SIEM] Use bulk actions API when updating or deleting rules (elastic#54521) Support "Deprecated" label in advanced settings (elastic#54539) [Maps] add text halo color and width style properties (elastic#53827) Service Map Data API at Runtime (elastic#54027) [SIEM] Detection Engine Create Rule Design Review #1 (elastic#54442) Skip flaky test [Canvas] Enable Embeddable maps (elastic#53971) [SIEM][Detection Engine] Increases the number or rules you can view on a single page (elastic#54628) uiSettings - use validation field for image field maxSize (elastic#54522) ...
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
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.
overall code LGTM, but made some comments, including a x ? y : z
expression where y
and z
were the same, which seems wrong
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, just had a question about the mergeBoolClauses
function and a nit about the console.log
); | ||
|
||
// call runNow for our task | ||
console.log(`... runnning ${originalTask.id}`); |
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.
nit: should the 3 console.log
calls be log.debug
?
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.
Nope, this is because I was working on fixing some flakyness in this test. All gone now.
organic: BoolClauseWithAnyCondition<T>; | ||
} | ||
|
||
export function mergeBoolClauses<T extends BoolClauseFilter>( |
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.
Doing a straight merge on queries that use a combination of should
with must
or must_not
can be dangerous if that's the case here.
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.
Same would apply with merging should
queries as I believe they are AND together instead of combined into a single OR.
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.
My thinking is that if someone is explicitly merging them, they mean for them to be applied as an AND, as it only merges the root object.
If someone wants an OR
they should use the shouldBeOneOf
.
We could rename mergeBoolClauses
to something that expresses this better, but not sure what it should be 🤔
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.
upon further inspection - this does merge deeply, my mistake, I'll simplify this.
Good catch!
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
As of Elasticsearch 8.0.0 it will no longer be possible to use the _id field on documents. This PR removes the usage that Task Manager makes of this field and switches to pinned queries to achieve a similar effect.
As of Elasticsearch 8.0.0 it will no longer be possible to use the _id field on documents. This PR removes the usage that Task Manager makes of this field and switches to pinned queries to achieve a similar effect.
Summary
As of Elasticsearch 8.0.0 it will no longer be possible to use the _id field on documents.
This PR removes the usage that Task Manager makes of this field and switches to pinned queries to achieve a similar effect.
closes #52517
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.This was checked for cross-browser compatibility, including a check against IE11Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportDocumentation was added for features that require explanation or tutorialsThis was checked for keyboard-only and screenreader accessibilityFor maintainers