-
Notifications
You must be signed in to change notification settings - Fork 2.3k
[Rule-based Auto-tagging] restructure the in-memory trie to store values as a set #19344
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
Conversation
d020ffb to
5550d32
Compare
jainankitk
left a 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.
Thanks @ruai0511 for breaking into smaller code changes. Mostly LGTM! Couple of minor comments to understand the code changes better
...utotagging-commons/common/src/main/java/org/opensearch/rule/storage/AttributeValueStore.java
Outdated
Show resolved
Hide resolved
...ing-commons/common/src/main/java/org/opensearch/rule/storage/DefaultAttributeValueStore.java
Outdated
Show resolved
Hide resolved
|
❌ Gradle check result for 5550d32: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
5550d32 to
00f931c
Compare
|
❌ Gradle check result for 9fa3af8: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
One of the failure looks related to this PR: @ruai0511 - Can you check this once? |
|
❌ Gradle check result for 9fa3af8: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Sorry which test is related? |
|
Oh yeah I already asked @Lindsay-00 yesterday to take a look since she's in charge of IT. |
fc729f0 to
58d35c6
Compare
...utotagging-commons/common/src/main/java/org/opensearch/rule/storage/AttributeValueStore.java
Outdated
Show resolved
Hide resolved
...utotagging-commons/common/src/main/java/org/opensearch/rule/storage/AttributeValueStore.java
Outdated
Show resolved
Hide resolved
...utotagging-commons/common/src/main/java/org/opensearch/rule/storage/AttributeValueStore.java
Show resolved
Hide resolved
...ing-commons/common/src/main/java/org/opensearch/rule/storage/DefaultAttributeValueStore.java
Outdated
Show resolved
Hide resolved
58d35c6 to
403d34b
Compare
|
❌ Gradle check result for 403d34b: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
Still seeing some flaky tests related to WLM: |
403d34b to
6433f37
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #19344 +/- ##
============================================
+ Coverage 72.81% 72.90% +0.09%
- Complexity 69838 69871 +33
============================================
Files 5674 5675 +1
Lines 320850 320886 +36
Branches 46383 46390 +7
============================================
+ Hits 233624 233945 +321
+ Misses 68361 67965 -396
- Partials 18865 18976 +111 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Ruirui Zhang <[email protected]>
6433f37 to
a36aaa4
Compare
|
❕ Gradle check result for a36aaa4: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
…pensearch-project#19344) Signed-off-by: Ruirui Zhang <[email protected]>
In this PR, we restructured the in-memory rule tries so that values are stored as a set instead of a single string.
The reason is that as we add more attributes into the system, a single attribute key may correspond to multiple valid labels.
For example:
These two rules are not duplicates—both should be stored. In the trie for index_pattern, the key a would map to:
By switching from a string to a set, we can correctly represent multiple labels for the same attribute key without overwriting or losing data.
More documentation on the feature is here: https://docs.opensearch.org/latest/tuning-your-cluster/availability-and-recovery/rule-based-autotagging/autotagging/
Description
[Describe what this change achieves]
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
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.