-
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] Transforms: Improves data view checks #181892
[ML] Transforms: Improves data view checks #181892
Conversation
77b0150
to
2e6abe5
Compare
Pinging @elastic/ml-ui (:ml) |
const dataViewTitle = Array.isArray(item.config.source.index) | ||
? item.config.source.index.join(',') | ||
: item.config.source.index; | ||
const dataViewId = getDataViewIdByTitle(dataViewTitle); | ||
const dataViewListItem = dataViewListItems.find((d) => d.title === dataViewTitle); |
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.
Each of the uses of dataViewListItems
runs a find
, these won't be too expensive, but it might be slightly more efficient if useGetDataViewIdsWithTitle
returned a map of data views, keyed on title
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 the hook to return a map, also renamed everything to be in line with the new returned structure in 9f23cb8.
x-pack/plugins/transform/public/app/hooks/use_get_data_views_title_id_map.ts
Outdated
Show resolved
Hide resolved
const toastNotifications = useToastNotifications(); | ||
|
||
const { getDataViewIdByTitle, loadDataViews } = useSearchItems(undefined); | ||
|
||
const { data: dataViewsTitleIdMap } = useGetDataViewsTitleIdMap(); |
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.
Total nit pick. but rather than having to rename data
-> dataViewsTitleIdMap
on every use, could it be renamed when being returned from useGetDataViewsTitleIdMap
?
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 hook wraps useQuery
so that whole returned object comes from it and data
is part of its standard API (like isLoading
, error
etc.).
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, LGTM.
Added a nit pick comment..
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
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @walterra |
Summary
Part of #181603.
For some of the actions in the transform list we need to identify if there's a data view for the target index. The way we identified this was quite inefficient. We had poor caching in place and fetched info for all data views including fields — this can be quite expensive queries!
This update fixes the approach and switches to using
dataViews.getIdsWithTitle()
in combination withuseQuery()
to bring the caching in line with how we load and refresh the rest of the transform list.Before: Lots of field caps requests!
After: We do just 1
_search
request that gets the data view ids/titles:Checklist