LakshSingla commented on code in PR #15015:
URL: https://github.com/apache/druid/pull/15015#discussion_r1332567880
##########
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:
I get the incorrect assumption I made in my original example. This
digression occurs from the semantics of how intervals are applied in load rules
v/s in drop rules. In load rules, we only need for partial overlap for the rule
to fire, while in the drop rule, we need a complete overlap for the rule to fire
However when we are doing a `contains` check in this logic, we are
inherently assuming something about the structure of the rules.
Consider the following case:
The system has 2 types of segments, one ranging from 2020-2021 and the other
from 2020-2022. If the user wants to drop the segments ranging from 2020-2021
while preserving the other segments, how does the user craft intuitive rules
for the same without causing the newly added check to fail? I only came up with
the following, however, that fails the check.
1. Drop 2020-2021
2. Load 2020-2022
I think this ties back to the common use case that we are trying to prevent
the user from doing?
--
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]
