-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[rule based autotagging] Add Get Rule API Logic #17336
base: main
Are you sure you want to change the base?
Conversation
d825a21
to
db57bcb
Compare
❌ Gradle check result for db57bcb: 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? |
db57bcb
to
f7bea2e
Compare
❌ Gradle check result for f7bea2e: 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? |
Signed-off-by: Ruirui Zhang <mariazrr@amazon.com>
f7bea2e
to
4d2d843
Compare
❌ Gradle check result for 4d2d843: 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? |
* A request for get Rule | ||
* @opensearch.experimental | ||
*/ | ||
public class GetRuleRequest extends ClusterManagerNodeRequest<GetRuleRequest> { |
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 we should not be extending this class from ClusterManagerNodeRequest
since this operation will be carried out on a data node.
private final String _id; | ||
private final Map<RuleAttribute, Set<String>> attributeFilters; | ||
|
||
/** |
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 add more details about the request and response behavior in the details of the PR ? For example what will be the structure of query payload and all. Can we confirm how other system indices support this behavior to keep the behavior uniform.
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.
System Indices like .opensearch_*, .monitoring, .tasks, .security, .ml, etc., generally support query parameters like filters, pagination (search_after), and sorting. But those are from endpoint that supports search operations (e.g., _search, _count, etc.), and will automatically handle those parameters. For us it's a bit different because we're implementing our own get api. At this point i think supporting search_after
and size
should be enough
* Response for the get API for Rule | ||
* @opensearch.experimental | ||
*/ | ||
public class GetRuleResponse extends ActionResponse implements ToXContent, ToXContentObject { |
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 add a sample response in the class level comment ?
if (matchesFilters(currRule, attributeFilters)) { | ||
return Map.entry(hit.getId(), currRule); | ||
} |
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.
we should pass these filters to the request itself so that the filtering happens at the lucene layer and we get more accurate results back rather than all the Rules from the index ?
private void fetchAllRules(Map<RuleAttribute, Set<String>> attributeFilters, ActionListener<GetRuleResponse> listener) { | ||
ThreadContext.StoredContext storedContext = client.threadPool().getThreadContext().stashContext(); | ||
client.prepareSearch(RULE_INDEX) | ||
.setQuery(QueryBuilders.matchAllQuery()) |
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 query should contain all the filters and query clauses, that will help us remove majority of the logic in this class
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 I modified the logic
* @opensearch.experimental | ||
*/ | ||
public class RulePersistenceService { | ||
public static final String RULE_INDEX = ".rule"; |
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 we should rename this to either .rules
or .rule_index
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.
Renamed to .rules because i think including the word 'index' in index name seems a bit redundant
Description
This PR introduces the get Rule API Logic.
An example API request is:
And the return would be
Rule Schema PR:
#17238
RFC:
#16797
Check List