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

Adding remote indices and multi index functionality #854

Merged
merged 2 commits into from
Sep 4, 2024

Conversation

amitgalitz
Copy link
Member

@amitgalitz amitgalitz commented Aug 29, 2024

Description

  1. Adding the ability to fetch information on remote clusters and select which clusters we want to choose datasources from
  2. Adds the ability to get information from remote indices such as the mapping
  3. Add the ability to select more the one index/alias/index-pattern
  4. Only resets the chosen model configuration if an index has been removed but not added

Issues Resolved

resolves #856
resolves #855

Check List

  • New functionality includes testing.
    • All tests pass
  • 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.

) => {
const newSelectedOptionsSet = new Set(newSelectedOptions);
const indexRemoved: boolean =
!(newSelectedOptions && oldSelectedOptions) ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

If both newSelectedOptions and oldSelectedOptions are null or undefined, would it make more sense for isSelectedOptionIndexRemoved to return false instead of true, because if both are null or undefined, there is no change in the selection state — technically, no option was "removed." ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Originally if both were null it was okay for me to set it to true because then it would also make sense to reset the model configuration if we can't find any selected indices.
I think actually though we pretty much never hit the case where both are undefined so it wasn't making too much of an effect and it logically makes more sense to say indexRemoved is false in that case and if we got to this point we would have had an indexRemoved previously.
I tested it out and removing !(newSelectedOptins && oldSelectedOptions) and adding:

    if (_.isEmpty(oldSelectedOptions) && _.isEmpty(newSelectedOptions)) {
      return false;
    }

also works

Copy link
Collaborator

Choose a reason for hiding this comment

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

make sense

@kaituo
Copy link
Collaborator

kaituo commented Aug 30, 2024

cypress test is failing. Can you either fix it or make sure it passes locally?

@amitgalitz
Copy link
Member Author

cypress test is failing. Can you either fix it or make sure it passes locally?

It does pass locally, I'm not sure why its failing here.

kaituo
kaituo previously approved these changes Aug 30, 2024
@kaituo
Copy link
Collaborator

kaituo commented Aug 31, 2024

Cypress test failed with a different reason now:

plugins/anomaly-detection-dashboard 03:56 4 3 1 - - │
│ s-plugin/vis_augmenter/view_anomaly │
│ _events_spec.js

@kaituo
Copy link
Collaborator

kaituo commented Sep 2, 2024

I think I fixed the timeout error during historical cypress tests in #859 and opensearch-project/opensearch-dashboards-functional-test#1537

Not sure about events_spec.js cypress failure above.

});
remoteMappings = convertFieldCapsToMappingStructure(fieldCapsResponse);
}
Object.assign(mappings, remoteMappings);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this Object.assign works fine on merging nested structure? Should we create a new map object instead of mutating the mappings variable directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was reading online that Object.assign is more efficient which can be important as we can have indices with very large mappings. I tested it on complicated nested structures

@jackiehanyang
Copy link
Collaborator

overall lgtm, just I see E2E CI is failing. Please look into that to make sure it won't be a RC build blocker

Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
@amitgalitz amitgalitz merged commit b26f8c3 into opensearch-project:main Sep 4, 2024
9 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-854-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b26f8c32489693d36ac5479c99a7d9f1d43ef01d
# Push it to GitHub
git push --set-upstream origin backport/backport-854-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-854-to-2.x.

@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.17 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.17 2.17
# Navigate to the new working tree
cd .worktrees/backport-2.17
# Create a new branch
git switch --create backport/backport-854-to-2.17
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b26f8c32489693d36ac5479c99a7d9f1d43ef01d
# Push it to GitHub
git push --set-upstream origin backport/backport-854-to-2.17
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.17

Then, create a pull request where the base branch is 2.17 and the compare/head branch is backport/backport-854-to-2.17.

amitgalitz added a commit to amitgalitz/anomaly-detection-dashboards-plugin that referenced this pull request Sep 4, 2024
…ct#854)

* Adding remote indices and multi index functionality

Signed-off-by: Amit Galitzky <amgalitz@amazon.com>

* changing removed index check

Signed-off-by: Amit Galitzky <amgalitz@amazon.com>

---------

Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 5, 2024
jackiehanyang pushed a commit that referenced this pull request Sep 5, 2024
(cherry picked from commit 4942f5f)

Co-authored-by: Amit Galitzky <amgalitz@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants