-
Notifications
You must be signed in to change notification settings - Fork 76
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
changes to prevent creating two detectors with same name #261
changes to prevent creating two detectors with same name #261
Conversation
Signed-off-by: Raj Chakravarthi <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #261 +/- ##
============================================
- Coverage 36.96% 36.75% -0.21%
Complexity 887 887
============================================
Files 179 179
Lines 7010 7055 +45
Branches 855 859 +4
============================================
+ Hits 2591 2593 +2
- Misses 4182 4225 +43
Partials 237 237
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Signed-off-by: Raj Chakravarthi <[email protected]>
d3877dc
to
8d3d36d
Compare
//if (detectorIndices.detectorIndexExists()) { | ||
if (false) { |
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.
This condition looks weird. Can we fix it
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.
accident, will revert
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 did that "if (false) {" to make sure it wasnt my code that caused a bad integration test @getsaurabh02, reverted
QueryBuilders.nestedQuery( | ||
"detector", | ||
QueryBuilders.boolQuery().must( | ||
QueryBuilders.matchQuery( |
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.
can we move this logic out to a helper method
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 bit difficult at the last moment as it uses listeners.
Signed-off-by: Raj Chakravarthi <[email protected]>
eca500e
to
a9b0534
Compare
will this work if 2 concurrent requests are made ? |
Signed-off-by: Raj Chakravarthi <[email protected]>
…h on name in TransportIndexDetectorAction Signed-off-by: Raj Chakravarthi <[email protected]>
Signed-off-by: Raj Chakravarthi <[email protected]>
* [Cypress] Fix cypress flaky tests for findings opensearch-project#242 Signed-off-by: Jovan Cvetkovic <[email protected]> * [Cypress] Fix cypress flaky tests for findings opensearch-project#242 Signed-off-by: Jovan Cvetkovic <[email protected]> * [Cypress] Fix cypress flaky tests for findings opensearch-project#242 Signed-off-by: Jovan Cvetkovic <[email protected]> * [Cypress] Fix cypress flaky tests for findings opensearch-project#242 Signed-off-by: Jovan Cvetkovic <[email protected]> Signed-off-by: Jovan Cvetkovic <[email protected]>
@sbcd90 , @jowg-amazon , can you please review as we need this change - should not have detectors with same name as it will be a confusing experience. |
Signed-off-by: Raj Chakravarthi [email protected]
Description
changes to prevent creating two detectors with same name
Updated PR with a new approach. This approach uses name as the id for detector and onus is not on elasticsearch to generate id for detector after this change.
Issues Resolved
(#207)
Check List
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.