-
Notifications
You must be signed in to change notification settings - Fork 4
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
[MDS] datasource selector added #29
Conversation
Signed-off-by: Kawika Avilla <[email protected]>
Signed-off-by: Kawika Avilla <[email protected]>
Signed-off-by: Kawika Avilla <[email protected]>
Signed-off-by: Kawika Avilla <[email protected]>
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 type improvements and some questions
useEffect(() => { | ||
const subscriptions = connectionsService.getSelectedConnection$().subscribe((connection) => { | ||
connectionsService.setSelectedConnection$(connection); | ||
}); | ||
return () => { | ||
subscriptions.unsubscribe(); | ||
}; | ||
}, [connectionsService]); |
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.
why is this getting connection and setting the same connection?
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.
good catch. whenever Discover page just incase the client switched off page i can still set it in the service. but tbh it might just be left over. will look again post merge
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.
didn't fully understand.. but i just tested this PR and this section is giving me Uncaught RangeError: Maximum call stack size exceeded
const resp = await context.core.savedObjects.client.find<DataSourceAttributes>({ | ||
type: 'data-source', | ||
fields, | ||
perPage: 10000, |
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.
should this paginate?
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.
it could. and probably should once we start adding more to this wrapper
title: string; | ||
endpoint?: string; | ||
installedPlugins?: string[]; | ||
auth?: any; |
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.
yeah it should. the original approach i wanted to go with is not requiring any imports from the data source plugin in case it didn't get loaded in browser (data_source.enabled: false) because i dont think they have any lazy loading over there.
i think my goal here is even if we want to take the work for the flint datasources stuff can live in this repo but extend what they have in data sources.
where we can properly lazy loaded and make it a lot easier to use and then eventually unify it with the ds mgmt plugin
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.
i see. i thought it wouldn't matter for types and import
/import type {xxx
would be removed when transpiled to js
Co-authored-by: Joshua Li <[email protected]>
Co-authored-by: Joshua Li <[email protected]>
Co-authored-by: Joshua Li <[email protected]>
Signed-off-by: Kawika Avilla <[email protected]>
Added data source selector that pulls from saved objects and also added a connections service and passing it with the dataframe.
Related to:
opensearch-project/OpenSearch-Dashboards#7157
With DQL (and Lucene):
With PPL (and SQL):