LakshSingla commented on code in PR #15015:
URL: https://github.com/apache/druid/pull/15015#discussion_r1331012916
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/Rule.java:
##########
@@ -50,4 +50,12 @@ public interface Rule
boolean appliesTo(Interval interval, DateTime referenceTimestamp);
void run(DataSegment segment, SegmentActionHandler segmentHandler);
+
+ /**
+ * Return an eligible interval from the reference timestamp. Implemntations
Review Comment:
```suggestion
* Return an eligible interval from the reference timestamp.
Implementations
```
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/Rule.java:
##########
@@ -50,4 +50,12 @@ public interface Rule
boolean appliesTo(Interval interval, DateTime referenceTimestamp);
void run(DataSegment segment, SegmentActionHandler segmentHandler);
+
+ /**
+ * Return an eligible interval from the reference timestamp. Implemntations
+ * must return a valid interval based on the rule type.
+ * @param referenceTimestamp base timestamp
+ * @return
Review Comment:
```suggestion
```
##########
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:
What's the behaviour in the following situation:
Rule#1 - Covers the interval 2020-2021
Rule#2 - Covers the interval 2021-2022
Rule#3 - Covers the interval 2020-2022
In this case `validateRules` would not throw an exception, however, from my
understanding, Rule#3 would never trigger.
If that's the case, we should handle these cases as well (probably by
maintaining a set of intervals that the rules seen so far cover and checking if
the next rule isn't completely overshadowed by the interval set seen so far)
--
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]
