-
Notifications
You must be signed in to change notification settings - Fork 73
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
update the AD settings for opensearch. #47
Merged
Merged
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
7996ed5
update the AD settings for opensearch.
2077548
Update settings name prefix from opensearch to plugins.
dfbbea6
Update the version of checkstyle to keep sync with opensearch.
9e398fd
Update the Opensearch from 1.0.0-beta1 to the main branch to take the
eb4b0ab
Merge branch 'opensearch-project:main' into main
spbjss 6e5c9e9
Merge branch 'main' of https://github.com/spbjss/anomaly-detection
8fec893
1. Update the opensearch, common-utils and job-scheduler version from…
40b4d99
Update the opensearch version from 1.0.0 to 1.0.0-rc1
45ffd32
Update the version informations.
5d368a6
Update the rc1 related version config
4325929
Update checkstyle version to 8.29 to follow opensearch 1.x
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 know @saratvemulapalli has been saying that we would want these to be
plugins.
just to avoid renaming again :) Care to 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.
I think it was miss on our side, we haven't updated the guidance. I would prefer to have
plugins
instead ofopensearch
.Can we update the doc: https://github.com/opensearch-project/opensearch-plugins/blob/main/CONVENTIONS.md#settings
Also let folks working on JobScheduler to take care of it as well.
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.
Do you mean we should change "opensearch.anomaly_detection.xxx" to "plugins.anomaly_detection.xxx"?
Sure, I will make that change.
Thank you for the comments!
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.
Yes please. Thanks Alex!
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.
And why would we not make this
anomaly_detection.max_anomaly_detectors
? That's what k-nn is doing, for example.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.
Definitely that works too. But I would love to have consistency across different pieces in the plugins.
And, we have decided to go with that as we have already discussed in the public:
opensearch-project/opensearch-plugins#13
https://discuss.opendistrocommunity.dev/t/thoughts-on-having-opensearch-identifier-in-all-things-plugins/5855
I am not opposed to either way but since we have taken a decision in the public, I would prefer to stick with it unless we see strong contentions to why that doesnt work. What do you think?
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.
A-OK by me to include
plugins
. opensearch-project/opensearch-plugins#31There 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.
Haha oncall :/, thanks for the change!
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.
Thank you very much for the input!
I have submitted a commits to update "opensearch" to "plugins"