-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[ML] Data Frame Analytics: Fix race condition and support for feature influence legacy format. #81123
[ML] Data Frame Analytics: Fix race condition and support for feature influence legacy format. #81123
Conversation
Pinging @elastic/ml-ui (:ml) |
...analytics/pages/analytics_exploration/components/outlier_exploration/outlier_exploration.tsx
Outdated
Show resolved
Hide resolved
|
||
setTableItems(docs); | ||
setStatus(INDEX_STATUS.LOADED); | ||
if (!options.didCancel) { |
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.
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.
Tested latest edits and LGTM
const options = { didCancel: false }; | ||
getIndexData(jobConfig, dataGrid, searchQuery, options); | ||
return () => { | ||
options.didCancel = true; |
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.
shall we create a custom hook, let's call it useSafeEffect
for instance, that wraps the callback with didCancel
check under the hood?
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.
yep that's something we could do in a follow up for 7.11 - for this 7.10 fix I wanted to touch as little code as possible that's why I went for duplicate code and no new utils. i'd also like to refactor getIndexData
a bit so we don't have to use the mutable object but just a plain variable.
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.
Tested latest commit and LGTM
@alvarezmelissa87 Added an error callout when both index patterns for source/dest index are missing, ready for review :) |
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.
Left a comment regarding classification/regression results page when there is no source/destination index pattern.
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.
Tested latest edits and LGTM ⚡
… influence legacy format. (elastic#81123) - Fixes a race condition where searches for data grid results with different parameters would return in different order with the wrong results on display. Fix uses a pattern to cancel useEffect callback for getIndexData(). - Fixes identifying pre 7.10 feature influence format for outlier detection and will display a callout on the results page with information for a workaround. - To fix identifying the legacy format, some cleanup of other code relating to the old format had to be done. The ml results object field is no longer treated as a "special" field for outlier detection and is treated and retrieved in the same way as other fields. - Adds an error callout if no Kibana index pattern is available for source/dest index.
… influence legacy format. (elastic#81123) - Fixes a race condition where searches for data grid results with different parameters would return in different order with the wrong results on display. Fix uses a pattern to cancel useEffect callback for getIndexData(). - Fixes identifying pre 7.10 feature influence format for outlier detection and will display a callout on the results page with information for a workaround. - To fix identifying the legacy format, some cleanup of other code relating to the old format had to be done. The ml results object field is no longer treated as a "special" field for outlier detection and is treated and retrieved in the same way as other fields. - Adds an error callout if no Kibana index pattern is available for source/dest index.
… influence legacy format. (#81123) (#81275) - Fixes a race condition where searches for data grid results with different parameters would return in different order with the wrong results on display. Fix uses a pattern to cancel useEffect callback for getIndexData(). - Fixes identifying pre 7.10 feature influence format for outlier detection and will display a callout on the results page with information for a workaround. - To fix identifying the legacy format, some cleanup of other code relating to the old format had to be done. The ml results object field is no longer treated as a "special" field for outlier detection and is treated and retrieved in the same way as other fields. - Adds an error callout if no Kibana index pattern is available for source/dest index.
… influence legacy format. (#81123) (#81276) - Fixes a race condition where searches for data grid results with different parameters would return in different order with the wrong results on display. Fix uses a pattern to cancel useEffect callback for getIndexData(). - Fixes identifying pre 7.10 feature influence format for outlier detection and will display a callout on the results page with information for a workaround. - To fix identifying the legacy format, some cleanup of other code relating to the old format had to be done. The ml results object field is no longer treated as a "special" field for outlier detection and is treated and retrieved in the same way as other fields. - Adds an error callout if no Kibana index pattern is available for source/dest index.
…arm-phase-to-formlib * 'master' of github.com:elastic/kibana: (55 commits) [UX] Fix map color variance and apply proper filter for extended stats (elastic#81106) [User Experience] Use EuiSelect for percentiles instead of SuperSelect (elastic#81082) [DOCS] Add link for monitoring ssl settings (elastic#81057) [test] Await loading indicator in monitoring test (elastic#81279) [ILM] Minor copy and link additions to cloud CTA for cold phase (elastic#80512) [Mappings editor] Add scaled_float and date_range comp integration tests (elastic#81287) [Discover] Deangularize context.app (elastic#80851) [O11y Overview] Add code to display/hide UX section when appropriate (elastic#80873) [Discover] Extend DiscoverNoResults component to show different message on error (elastic#79671) Fix tagcloud word overlapping (elastic#81161) [Security Solution] Fixes flaky test rules (elastic#81040) Changed the code to avoid tech debt with hacky solutions after receiving comments on EUI issue reported about this problem. (elastic#81183) [Security Solution][All] Replace old markdown renderer with the new one (elastic#80301) Add namespaced version of the API call (elastic#81278) [ML] Data Frame Analytics: Fix race condition and support for feature influence legacy format. (elastic#81123) [Fleet] Fix POLICY_CHANGE action creation for new policy (elastic#81236) [Security Solution][Endpoint][Admin] Malware user notification checkbox (elastic#78084) [SecuritySolution][Unit Tests] - fix flakey unit test (elastic#81239) skip flaky suite (elastic#81264) [Maps] fix top-level Map page is called 'Kibana' (elastic#81238) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/hot_phase/hot_phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared/forcemerge_field.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/warm_phase.tsx
💚 Build SucceededMetrics [docs]async chunks size
History
To update your PR or re-run it, just comment with: |
Summary
Fixes #81150
Part of #77046
useEffect
callback forgetIndexData()
.ml
results object field is no longer treated as a "special" field for outlier detection and is treated and retrieved in the same way as other fields.Checklist