abhishekrb19 commented on code in PR #15015:
URL: https://github.com/apache/druid/pull/15015#discussion_r1332727568
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java:
##########
@@ -43,4 +48,40 @@ public static boolean eligibleForLoad(Period period,
Interval interval, DateTime
private Rules()
{
}
+
+ /**
+ * Validate rules. This method throws an exception if a rule contain an
interval that
+ * will fully cover the next rules' interval in the list. Rules that will be
evaluated at some point
+ * are considered to be legitimate.
+ * @param rules Datasource rules.
+ */
+ public static void validateRules(final List<Rule> rules)
+ {
+ if (rules == null) {
+ return;
+ }
+ final DateTime now = DateTimes.nowUtc();
+ for (int i = 0; i < rules.size() - 1; i++) {
+ final Rule currRule = rules.get(i);
+ final Rule nextRule = rules.get(i + 1);
+ final Interval currInterval = currRule.getEligibleInterval(now);
+ final Interval nextInterval = nextRule.getEligibleInterval(now);
+ if (currInterval.contains(nextInterval)) {
+ // If the current rule has an eternity interval, it covers everything
following it.
+ // Or if the current rule still covers the next rule at the current
interval boundaries, then the
+ // next rule will never fire at any time, so throw an exception.
+ if (Intervals.ETERNITY.equals(currInterval) ||
+
(currRule.getEligibleInterval(currInterval.getStart()).contains(nextRule.getEligibleInterval(currInterval.getStart()))
+ &&
currRule.getEligibleInterval(currInterval.getEnd()).contains(nextRule.getEligibleInterval(currInterval.getEnd()))))
{
+ throw InvalidInput.exception(
+ "Rule[%s] has an interval that contains interval for rule[%s].
The interval[%s] also covers interval[%s].",
+ currRule,
+ nextRule,
+ currInterval,
+ nextInterval
Review Comment:
@kfaraz - please see my responses to your comments:
> we would have to ensure that the fully covered check is done correctly.
To check if an interval fully contains another, the proposed algorithm has a
nested loop that goes through the sequence of rules to find if a larger
interval contains another subsequent interval down in the list; O(n^2)
algorithmic runtime, but practical runtime should be trivial though.
I suppose there are alternate approaches like condensing intervals - the
additive and subtractive logic you're describing. My first thought is that this
could work well for the overlapping case (which isn't the intent of this patch).
Re this:
> Secondly, all of this is easy to follow when we are dealing with
ForeverXyzRule or IntervalXyzRule (rule with fixed interval). For
PeriodXyzRule, I am not entirely sure of the boundaries that would need to be
checked. The proposed change does compute the eligibleInterval at both start
and end of the currInterval but I need to give it some more thought.
The boundary check primarily exists to allow the case where a rule will fire
at some point. Unlike the forever and interval rules, period rules factor in
the reference timestamp thereby yielding distinct intervals at the boundaries.
Something like:
`PeriodRuleR3.eligibleInterval(now) = IntervalR3`
`PeriodRuleR3.eligibleInterval(1yAgo) = IntervalR4`
The boundaries check is essentially to verify that a legit rule will fire at
some point and allow such a rule. For example, consider the rules
[`loadByPeriod(P10Y)`, `loadByInterval('2020/2025')`]:
- With the boundary check (before and after 10 years from now), the first
rule will _not_ contain the second interval. So the validation check will not
flag the rule set. The second rule will indeed fire at some point in the future.
- Without the boundary check, the first rule will fully cover the second
rule's interval, incorrectly flagging the second rule.
I realize I need to update the javadocs. :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
