-
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
[ML] Adding job sychronization warning to job list pages #85794
[ML] Adding job sychronization warning to job list pages #85794
Conversation
initSavedObjects(true) | ||
.then(({ jobs }) => { | ||
const missingJobs = jobType === undefined || jobs.some(({ type }) => type === jobType); | ||
setShowWarning(missingJobs); |
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.
this effect potentially can update the state of an unmounted component
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.
Isn't this the case for all async calls that are made from a useEffect
?
Normally we call an async function but do not await on it as you can't pass an async function to useEffect
, here I decided to use a then
rather than create a second function.
This is a pattern i've seen in plenty of places, is there a better alternative?
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.
Check out this example
, I don't know if there is a better wayThere 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.
ok yeah, a lock like this is a good solution.
We have a lot of components which have this potential problem.
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.
updated in 4eedc54
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.
yes, you're right, I see a warning message from React on this issue quite often.
x-pack/plugins/ml/public/application/components/saved_objects_warning/saved_objects_warning.tsx
Show resolved
Hide resolved
Pinging @elastic/ml-ui (:ml) |
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
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
@elasticmachine merge upstream |
@elasticmachine merge upstream |
retest |
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 ⚡
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
…6088) * [ML] Adding job sychronization warning to job list pages * fixing overview page callout * improving check logic * adding render lock Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
When loading the Overview, Anomaly Detection and Data Frame Analytics pages, a synchronization check is run to see if any jobs are missing their save objects.
A callout is then displayed with a link to the stack management page so the user can repair them,
Checklist
Delete any items that are not applicable to this PR.
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
This was checked for breaking API changes and was labeled appropriately