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.

Sign up for GitHub

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

Jump to bottom

added fix for jobs and cache Support for workbench ,MDS support #1739

Merged
merged 10 commits into from
May 1, 2024
Merged

added fix for jobs and cache Support for workbench ,MDS support #1739

merged 10 commits into from
May 1, 2024

Conversation

sumukhswamy
Copy link
Collaborator

@sumukhswamy sumukhswamy commented Apr 25, 2024 •
edited
Loading

Description

added changes for cache loader, query workbench routes and mds support

Issues Resolved

link to previous PR
#1735

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@sumukhswamy sumukhswamy changed the base branch from mds-update-tocache to main April 25, 2024 01:10
@sumukhswamy sumukhswamy changed the title addressed PR comments, added fix for jobs and cache added fix for jobs and cache Support for workbench Apr 25, 2024
@sumukhswamy sumukhswamy changed the title added fix for jobs and cache Support for workbench added fix for jobs and cache Support for workbench ,MDS support Apr 25, 2024
ps48
ps48 reviewed Apr 26, 2024
View reviewed changes
public/framework/catalog_cache/cache_manager.test.tsx Outdated
const defaultCacheObject = {
version: CATALOG_CACHE_VERSION,
dataSources: [],
dataSourceMDSId: '',
Copy link
Member

Choose a reason for hiding this comment

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

It is better to not change older tests to maintain BWC. Rather add new tests for new changes, the older ones should work as is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, have updated the cache object now it only takes in the id if its present

ps48
ps48 reviewed Apr 26, 2024
View reviewed changes
public/services/requests/sql.ts
return this.http
.post('/api/observability/query/jobs', {
body: JSON.stringify(params),
query,
Copy link
Member

Choose a reason for hiding this comment

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

We should merge the dataSourceMDSId in post request body, similar to

dashboards-observability/server/routes/custom_panels/panels_router.ts

Lines 338 to 344 in 3dbe5c8

body: schema.object({
panelId: schema.string(),
query: schema.string(),
language: schema.string(),
to: schema.string(),
from: schema.string(),
}),
and update this router accordingly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wanted the dataSourceMds id to be visible for the api call that is why it is outside

mengweieric
mengweieric reviewed Apr 26, 2024
View reviewed changes
public/framework/catalog_cache/cache_manager.ts Outdated
@@ -68,27 +74,34 @@ export class CatalogCacheManager {
* Retrieves accelerations cache from local storage.
* @returns {AccelerationsCacheData} The retrieved accelerations cache.
*/
static getAccelerationsCache(): AccelerationsCacheData {
static getAccelerationsCache(dataSourceMDSId?: string): AccelerationsCacheData {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like we want to provide a default empty '' when there's no dataSourceMDSId passed. Then you don't have to check existance as well as using '|| '' ' in the following code.

public/framework/catalog_cache/cache_manager.ts Outdated
this.saveAccelerationsCache(defaultCacheObject);
return defaultCacheObject;
const cachedAcclerationData = JSON.parse(accelerationCacheData);
if (dataSourceMDSId && cachedAcclerationData.dataSourceMDSId === dataSourceMDSId) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need to trim 'dataSourceMDSId' ? what if value passed in is ' MDSId-123 ' and the actual id is 'MDSId-123'?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I dont think we are adding a space to the id anywhere, could you clarify from where the space is?

joshuali925
joshuali925 reviewed Apr 26, 2024
View reviewed changes
common/utils/shared.ts Show resolved Hide resolved
public/framework/catalog_cache/cache_loader.tsx
@@ -253,7 +255,7 @@ export const useLoadToCache = (loadCacheType: LoadCacheType) => {
startPolling,
stopPolling: stopLoading,
} = usePolling<any, any>((params) => {
return sqlService.fetchWithJobId(params);
return sqlService.fetchWithJobId(params, dataSourceMDSClientId.current);
Copy link
Member

Choose a reason for hiding this comment

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

does this still trigger request if dataSourceMDSId is undefined and dataSourceMDSClientId.current is empty string?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup

Copy link
Collaborator

Choose a reason for hiding this comment

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

do we want to trigger it when it's undefined / empty string?

public/framework/catalog_cache/cache_manager.ts Outdated Show resolved Hide resolved
public/framework/catalog_cache/cache_manager.ts Outdated Show resolved Hide resolved
@sumukhswamy sumukhswamy added enhancement New feature or request backport 2.x labels Apr 30, 2024
ps48
ps48 reviewed Apr 30, 2024
View reviewed changes
public/framework/catalog_cache/cache_loader.tsx
databases: [],
lastUpdated: currentTime,
status: CachedDataSourceStatus.Failed,
...(dataSourceMDSId && { dataSourceMDSId }),
Copy link
Member

@ps48 ps48 Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@sumukhswamy Let's replace this with if logic at other places too.

@sumukhswamy sumukhswamy merged commit 868ddcb into opensearch-project:main May 1, 2024
10 of 21 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 1, 2024
@github-actions
added fix for jobs and cache Support for workbench ,MDS support (#1739)
edb76e7 
* cacahe updated to support MDS-client and added MDS support

Signed-off-by: sumukhswamy <[email protected]>

* addressed PR comments, added fix for jobs and cache

Signed-off-by: sumukhswamy <[email protected]>

* updated the tests, PR comments

Signed-off-by: sumukhswamy <[email protected]>

* addressed comments, fixed loading of flint datasources

Signed-off-by: sumukhswamy <[email protected]>

* added fix for cache and cahnged router

Signed-off-by: sumukhswamy <[email protected]>

* fixed linter

Signed-off-by: sumukhswamy <[email protected]>

---------

Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: Sumukh Swamy <[email protected]>
(cherry picked from commit 868ddcb)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request May 1, 2024
Merged
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 1, 2024
@github-actions
added fix for jobs and cache Support for workbench ,MDS support (#1739)
d199193 
* cacahe updated to support MDS-client and added MDS support

Signed-off-by: sumukhswamy <[email protected]>

* addressed PR comments, added fix for jobs and cache

Signed-off-by: sumukhswamy <[email protected]>

* updated the tests, PR comments

Signed-off-by: sumukhswamy <[email protected]>

* addressed comments, fixed loading of flint datasources

Signed-off-by: sumukhswamy <[email protected]>

* added fix for cache and cahnged router

Signed-off-by: sumukhswamy <[email protected]>

* fixed linter

Signed-off-by: sumukhswamy <[email protected]>

---------

Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: Sumukh Swamy <[email protected]>
(cherry picked from commit 868ddcb)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request May 1, 2024
Merged
sumukhswamy pushed a commit that referenced this pull request May 1, 2024
@opensearch-trigger-bot @github-actions
added fix for jobs and cache Support for workbench ,MDS support (#1739…
fdf2808 
…) (#1784)

* cacahe updated to support MDS-client and added MDS support



* addressed PR comments, added fix for jobs and cache



* updated the tests, PR comments



* addressed comments, fixed loading of flint datasources



* added fix for cache and cahnged router



* fixed linter



---------



(cherry picked from commit 868ddcb)

Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: Sumukh Swamy <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
sumukhswamy pushed a commit that referenced this pull request May 1, 2024
@opensearch-trigger-bot @github-actions
added fix for jobs and cache Support for workbench ,MDS support (#1739…
1c6786d 
…) (#1783)

* cacahe updated to support MDS-client and added MDS support



* addressed PR comments, added fix for jobs and cache



* updated the tests, PR comments



* addressed comments, fixed loading of flint datasources



* added fix for cache and cahnged router



* fixed linter



---------



(cherry picked from commit 868ddcb)

Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: Sumukh Swamy <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@RyanL1997 RyanL1997 mentioned this pull request May 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhyuanqi zhyuanqi zhyuanqi left review comments

@joshuali925 joshuali925 joshuali925 left review comments

@BionIT BionIT BionIT left review comments

@ps48 ps48 ps48 approved these changes

@mengweieric mengweieric mengweieric approved these changes

@kavithacm kavithacm Awaiting requested review from kavithacm

@derek-ho derek-ho Awaiting requested review from derek-ho

@dai-chen dai-chen Awaiting requested review from dai-chen

@YANG-DB YANG-DB Awaiting requested review from YANG-DB

@rupal-bq rupal-bq Awaiting requested review from rupal-bq

@vamsimanohar vamsimanohar Awaiting requested review from vamsimanohar

@Swiddis Swiddis Awaiting requested review from Swiddis

@penghuo penghuo Awaiting requested review from penghuo

@seankao-az seankao-az Awaiting requested review from seankao-az

@anirudha anirudha Awaiting requested review from anirudha

@paulstn paulstn Awaiting requested review from paulstn

Assignees
No one assigned
Labels
backport 2.x backport 2.14 enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sumukhswamy @ps48 @BionIT @joshuali925 @zhyuanqi @mengweieric