Skip to content

Conversation

@opensearch-trigger-bot
Copy link
Contributor

Backport 8932876 from #17336.

* add get rule api logic
Signed-off-by: Ruirui Zhang <[email protected]>

* modify based on comments
Signed-off-by: Ruirui Zhang <[email protected]>

* rebase from main after the schema merged
Signed-off-by: Ruirui Zhang <[email protected]>

* modify based on comments
Signed-off-by: Ruirui Zhang <[email protected]>

* extract common logics to libs
Signed-off-by: Ruirui Zhang <[email protected]>

* Add javadocs for libs
Signed-off-by: Ruirui Zhang <[email protected]>

* modify based on comments
Signed-off-by: Ruirui Zhang <[email protected]>

* modify based on comments
Signed-off-by: Ruirui Zhang <[email protected]>

* modify based on comments
Signed-off-by: Ruirui Zhang <[email protected]>

* correct UT
Signed-off-by: Ruirui Zhang <[email protected]>

* modify based on comments
Signed-off-by: Ruirui Zhang <[email protected]>

r

* refactor code and fix ut

Signed-off-by: Kaushal Kumar <[email protected]>

* remove commented code

Signed-off-by: Kaushal Kumar <[email protected]>

* address comments

Signed-off-by: Kaushal Kumar <[email protected]>

* change method name

Signed-off-by: Kaushal Kumar <[email protected]>

* fix merge conflicts

Signed-off-by: Kaushal Kumar <[email protected]>

* rename queryGroup to workloadGroup

Signed-off-by: Kaushal Kumar <[email protected]>

* add guice binding related changes

Signed-off-by: Kaushal Kumar <[email protected]>

* refactor code to create a generic rule framework structure

Signed-off-by: Kaushal Kumar <[email protected]>

* fix javadoc
Signed-off-by: Ruirui Zhang <[email protected]>

* fix UT
Signed-off-by: Ruirui Zhang <[email protected]>

* restructure tests

Signed-off-by: Kaushal Kumar <[email protected]>

* rebase with mainline

Signed-off-by: Kaushal Kumar <[email protected]>

* fix gradlew file
Signed-off-by: Ruirui Zhang <[email protected]>

* add UT
Signed-off-by: Ruirui Zhang <[email protected]>

* add action UTs

Signed-off-by: Kaushal Kumar <[email protected]>

* correct the comment

Signed-off-by: Kaushal Kumar <[email protected]>

* add more UT
Signed-off-by: Ruirui Zhang <[email protected]>

---------

Signed-off-by: Kaushal Kumar <[email protected]>
Co-authored-by: Kaushal Kumar <[email protected]>
(cherry picked from commit 8932876)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Copy link
Member

@andrross andrross left a comment

Choose a reason for hiding this comment

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

The 3.0 beta was the cutoff for new features, so I think we'll want to be pretty selective about what we merge into 3.0 at this point to ensure a stable release. This looks to me like much more of a new feature rather than a fix for a bug or performance issue. Is this critical to bring into 3.0 or can it be deferred until 3.1?

@github-actions
Copy link
Contributor

✅ Gradle check result for ec03428: SUCCESS

@codecov
Copy link

codecov bot commented Apr 23, 2025

Codecov Report

Attention: Patch coverage is 73.52941% with 54 lines in your changes missing coverage. Please review.

Please upload report for BASE (3.0@816787d). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...va/org/opensearch/rule/rest/RestGetRuleAction.java 9.09% 30 Missing ⚠️
.../java/org/opensearch/rule/RuleFrameworkPlugin.java 42.85% 8 Missing ⚠️
...ule/service/IndexStoredRulePersistenceService.java 80.00% 4 Missing and 3 partials ⚠️
...rg/opensearch/rule/storage/XContentRuleParser.java 70.00% 3 Missing ⚠️
...arch/plugin/wlm/rule/WorkloadGroupFeatureType.java 77.77% 2 Missing ⚠️
...main/java/org/opensearch/rule/GetRuleResponse.java 95.00% 0 Missing and 1 partial ⚠️
...a/org/opensearch/rule/autotagging/FeatureType.java 0.00% 1 Missing ⚠️
...search/rule/storage/IndexBasedRuleQueryMapper.java 94.11% 0 Missing and 1 partial ⚠️
...wlm/rule/attribute_extractor/IndicesExtractor.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##             3.0   #18055   +/-   ##
======================================
  Coverage       ?   72.58%           
  Complexity     ?    67152           
======================================
  Files          ?     5473           
  Lines          ?   309913           
  Branches       ?    45025           
======================================
  Hits           ?   224961           
  Misses         ?    66597           
  Partials       ?    18355           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jainankitk
Copy link
Contributor

The 3.0 beta was the cutoff for new features, so I think we'll want to be pretty selective about what we merge into 3.0 at this point to ensure a stable release. This looks to me like much more of a new feature rather than a fix for a bug or performance issue. Is this critical to bring into 3.0 or can it be deferred until 3.1?

Thanks @andrross for bringing this up. While some of it is new feature, this PR has refactoring for moving autotagging logic into modules, which is the right place IMO. Hence, I would like to get this in to ensure we don't pollute core with stuff that is not needed!

@andrross
Copy link
Member

andrross commented Apr 23, 2025

While some of it is new feature, this PR has refactoring for moving autotagging logic into modules

@jainankitk This commit contains mostly new files. I don't see any files that have moved? Nevermind, I see now that some files moved from a lib to a module.

Hence, I would like to get this in to ensure we don't pollute core with stuff that is not needed!

I think you've accomplished that goal by committing this change to main. Why does this need to be backported into the release?

@kaushalmahi12
Copy link
Contributor

This change should not be a breaking change since core functionality is still dorment (It is behind a plugin). Given we want to introduce the auto-tagging feature starting 3.0, I think it should be fine to put this in 3.0.

@jainankitk
Copy link
Contributor

I think you've accomplished that goal by committing this change to main. Why does this need to be backported into the release?

I was hoping to make this change eagerly, but if you don't see any concerns with this happening in 3.1 instead of 3.0, I am fine with that.

@jainankitk jainankitk closed this Apr 23, 2025
@github-actions github-actions bot deleted the backport/backport-17336-to-3.0 branch April 23, 2025 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants