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

[BUG] Updated cache for the sub tree in Workbench #2351

Merged
merged 7 commits into from
Feb 20, 2025

Conversation

sumukhswamy
Copy link
Collaborator

@sumukhswamy sumukhswamy commented Feb 12, 2025

Description

Side tree flyout fix for the caching mechanism

Screen.Recording.2025-02-12.at.3.04.25.PM.mov

Issues Resolved

opensearch-project/dashboards-query-workbench#445

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.

Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: sumukhswamy <[email protected]>
@sumukhswamy sumukhswamy added bug Something isn't working v2.20.0 labels Feb 12, 2025
@sumukhswamy sumukhswamy changed the title [BUG]Flyout fix [BUG] Updated cache for the sub tree in Workbench Feb 12, 2025
const getMappings = (
index: string,
dataSourceMDSId?: string
): Promise<OpenSearchDashboardsResponse> | undefined => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

return Promise or undefined? is it supposed to be Promise<OpenSearchDashboardsResponse | undefined>?

@mengweieric
Copy link
Collaborator

does query workbench depend on this change? Why query workbench changes need to modify Observability? Could you elaborate more in description?

@sumukhswamy
Copy link
Collaborator Author

so the workbench side tree in case of flint depends on the caching mechanism from observability thats why this PR has been added to update a few conditions in the caching

@mengweieric
Copy link
Collaborator

may need to update snapshots: 19 snapshots failed.

@@ -28,9 +28,9 @@ export default class DSLService {
.catch((error) => console.error(error));
};

fetchIndices = async (index: string = '') => {
fetchIndices = async (index: string = '', dataSourceMDSId?: string) => {
Copy link
Member

Choose a reason for hiding this comment

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

not sure how this is handled when MDS is diabled. should we have a default value here?
dataSourceMDSId = ''

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

have tested it, works when is the id is empty as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will update it to '' when MDS is disabled

Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a place that when mds is disabled we set mds id to ''? Or we should just handle the undefined instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

expect(coreRefsModule.coreRefs.dslService!.fetchIndices).toHaveBeenCalledWith('testIndex');
expect(coreRefsModule.coreRefs.dslService!.fetchFields).toHaveBeenCalledWith('testIndex', '');
expect(coreRefsModule.coreRefs.dslService!.fetchSettings).toHaveBeenCalledWith('testIndex', '');
expect(coreRefsModule.coreRefs.dslService!.fetchIndices).toHaveBeenCalledWith('testIndex', '');
Copy link
Member

Choose a reason for hiding this comment

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

May be add test cases with MDS enabled as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay sure

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

test added

Copy link
Collaborator

@RyanL1997 RyanL1997 left a comment

Choose a reason for hiding this comment

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

Hi @sumukhswamy , thanks for putting this together, and here are some comments/questions.

@@ -28,9 +28,9 @@ export default class DSLService {
.catch((error) => console.error(error));
};

fetchIndices = async (index: string = '') => {
fetchIndices = async (index: string = '', dataSourceMDSId?: string) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a place that when mds is disabled we set mds id to ''? Or we should just handle the undefined instead?

@sumukhswamy sumukhswamy merged commit 535fa86 into opensearch-project:main Feb 20, 2025
18 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Feb 20, 2025
* updated the flyout fix

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

* updated the flyout fix

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

* updated snapshots

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

* Added test for accleration details flyout with MDSId

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

---------

Signed-off-by: sumukhswamy <[email protected]>
(cherry picked from commit 535fa86)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
sumukhswamy pushed a commit that referenced this pull request Feb 20, 2025
* updated the flyout fix



* updated the flyout fix



* updated snapshots



* Added test for accleration details flyout with MDSId



---------


(cherry picked from commit 535fa86)

Signed-off-by: sumukhswamy <[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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x bug Something isn't working v2.20.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants