Skip to content
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] Fix race condition when updating data grid row count #149518

Merged
merged 3 commits into from
Jan 27, 2023

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Jan 25, 2023

Summary

Part of #71231.

This fixes an edges case where handling the row count information (the actual count and the relation (eq/gte)) could trigger two independent state updates. Because of those multiple state updates the check whether to show the mini histograms by default could fail. For example updating from count: 0, relation: eq to count: 10000, relation: gte could cause an intermediate state count: 10000, relation, eq which would pass the check to show the mini charts.

The fix in this PR is to combine the two settings to use only one useState().

Functional tests that would catch this bug are part of the date picker update for transforms (#149049), I'm just breaking this fix out into a separate PR for to make reviewing easier/more focused.

Checklist

@walterra walterra added bug Fixes for quality problems that affect the customer experience :ml release_note:skip Skip the PR/issue when compiling release notes v8.7.0 labels Jan 25, 2023
@walterra walterra self-assigned this Jan 25, 2023
@walterra walterra mentioned this pull request Jan 25, 2023
13 tasks
@walterra walterra force-pushed the ml-data-grid-fix-row-count branch from 0617bda to 2366558 Compare January 25, 2023 14:37
@walterra walterra marked this pull request as ready for review January 25, 2023 15:54
@walterra walterra requested a review from a team as a code owner January 25, 2023 15:54
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@qn895
Copy link
Member

qn895 commented Jan 26, 2023

Code LGTM 🎉

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ml 3.4MB 3.4MB +38.0B
transform 348.0KB 348.1KB +64.0B
total +102.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @walterra

@walterra walterra merged commit 0174705 into elastic:main Jan 27, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 27, 2023
@walterra walterra deleted the ml-data-grid-fix-row-count branch January 27, 2023 10:07
kqualters-elastic pushed a commit to kqualters-elastic/kibana that referenced this pull request Feb 6, 2023
…9518)

This fixes an edges case where handling the row count information (the
actual count and the relation (`eq/gte`)) could trigger two independent
state updates. Because of those multiple state updates the check whether
to show the mini histograms by default could fail. For example updating
from `count: 0, relation: eq` to `count: 10000, relation: gte` could
cause an intermediate state `count: 10000, relation, eq` which would
pass the check to show the mini charts. The fix here is to combine the
two settings to use only one `useState()`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting bug Fixes for quality problems that affect the customer experience :ml release_note:skip Skip the PR/issue when compiling release notes v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants