[Workload Management] Add rule schema for workload management #17238
Conversation
ruai0511
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
cwperks,
dblock,
dbwiddis,
gbbafna,
jainankitk,
kotwanikunal,
linuxpi,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami and
VachaShah
as code owners
ruai0511
force-pushed
the
rule-schema-PR
branch
from
d2ec3f2 to
bc12186
Compare
Signed-off-by: Ruirui Zhang <[email protected]>
ruai0511
force-pushed
the
rule-schema-PR
branch
from
bc12186 to
f6a4a28
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17238 +/- ##
============================================
+ Coverage 72.36% 72.44% +0.07%
- Complexity 65733 65825 +92
============================================
Files 5318 5319 +1
Lines 305674 305812 +138
Branches 44349 44376 +27
============================================
+ Hits 221197 221535 +338
+ Misses 66341 66128 -213
- Partials 18136 18149 +13 ☔ View full report in Codecov by Sentry. |
| * } | ||
| */ | ||
| @ExperimentalApi | ||
| public class Rule extends AbstractDiffable<Rule> implements ToXContentObject { |
There was a problem hiding this comment.
I think We don't need to send the diffs of this entity over wire or deal with diffs at all. Is there any other reason for making the entity AbstractDiffable ?
There was a problem hiding this comment.
We need to override the writeTo function so need this class.
There was a problem hiding this comment.
For that there is Writeable interface. The javadoc comment for the class is very clear about its use.
| import static org.opensearch.cluster.metadata.QueryGroup.isValid; | ||
|
|
||
| /** | ||
| * Class to define the Rule schema |
There was a problem hiding this comment.
Can we add more details about the objective of the class and how does it fit in the ecosystem such as preconditions for this class such as
- Rule based auto tagging is enabled
- Only capture the in memory view of the Rule but the indexed view might be little different
There was a problem hiding this comment.
'Only capture the in memory view of the Rule but the indexed view might be little different' - will this be more suitable to put in the Trie class? since a trie captures the in memory view of the Rule
There was a problem hiding this comment.
I think since this is temp representation of Rule since it will only be employed when parsing the RestRequest into Transport one. Hence It should convey that but yes it will translate to multiple attribute based tries for in memory based representation
| */ | ||
| @ExperimentalApi | ||
| public class Rule extends AbstractDiffable<Rule> implements ToXContentObject { | ||
| private final String _id; |
There was a problem hiding this comment.
Do we need the _id field here ? I think the id will be generated by opensearch indexing code path automatically
| private final Feature feature; | ||
| private final String label; | ||
| private final String updatedAt; | ||
| public static final Map<Feature, Set<RuleAttribute>> featureAlloedAttributesMap = Map.of( |
There was a problem hiding this comment.
[NIT] typo in the constant name. we should name the constants in uppercase to distinguish it from variables.
| * For example, if we're creating a rule for WLM/QueryGroup, the rule will contain the line | ||
| * "query_group": "query_group_id", | ||
| * so the feature name would be "query_group" in this case. |
There was a problem hiding this comment.
We can remove this comment
There was a problem hiding this comment.
I added this comment to let users know what is a feature. Don't want them to get confused because i think the name is not so straightforward. But i guess a better place to explain this should be in the user documentation. I removed this
| builder.array(entry.getKey().getName(), entry.getValue().toArray(new String[0])); | ||
| } | ||
| builder.field(feature.getName(), label); | ||
| builder.field("updated_at", updatedAt); |
There was a problem hiding this comment.
We should replace the string and numeric literals with well defined constants as such code changes are prone to errors when the literal value needs to change
There was a problem hiding this comment.
Dang won't happen again
| public static Diff<Rule> readDiff(final StreamInput in) throws IOException { | ||
| return readDiffFrom(Rule::new, in); | ||
| } |
There was a problem hiding this comment.
I think it should not be required
| public Builder builderFromRule() { | ||
| return new Builder()._id(_id).label(label).feature(feature.getName()).updatedAt(updatedAt).attributeMap(attributeMap); | ||
| } |
There was a problem hiding this comment.
This looks odd and misplaced I think this looks a lot like builder.build(). Any particular reason for this ?
There was a problem hiding this comment.
I previously added this as a helper function to change the rule id to the index generated id. But since we no longer have the _id field in a Rule schema we no longer need this function. I removed it.
| } | ||
| } else if (token == XContentParser.Token.START_ARRAY) { | ||
| RuleAttribute ruleAttribute = RuleAttribute.fromName(fieldName); | ||
| Set<String> indexPatternList = new HashSet<>(); |
There was a problem hiding this comment.
This var name is coupled to a singled attribute based values, we should avoid that as this can easily create confusion. If there are other Rule attribute names then this is a misnomer
Signed-off-by: Ruirui Zhang <[email protected]>
ruai0511
force-pushed
the
rule-schema-PR
branch
from
094c95d to
bb8d06c
Compare
|
❕ Gradle check result for bb8d06c: 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. |
Description
This PR introduces the schema for Rule object used in the workload management feature
Related issues:
RFC: #16797
#16813
Check List