From f72eda008b5c3880ec5d0ff878b76dc5a9f69147 Mon Sep 17 00:00:00 2001 From: Abhishek Balaji Radhakrishnan Date: Tue, 19 Sep 2023 14:27:34 -0700 Subject: [PATCH 1/9] Disallow overshadowing rules in rule chain. This would prevent users from shooting themselves in the foot. For example, if an interval in a rule is "large enough" and covers an interval in subsequent rule(s), then those rules will never run. Implement guard rails for rules in the rule chain. --- .../ForeverBroadcastDistributionRule.java | 12 + .../coordinator/rules/ForeverDropRule.java | 12 + .../coordinator/rules/ForeverLoadRule.java | 14 + .../IntervalBroadcastDistributionRule.java | 14 + .../coordinator/rules/IntervalDropRule.java | 14 + .../coordinator/rules/IntervalLoadRule.java | 19 +- .../PeriodBroadcastDistributionRule.java | 15 + .../rules/PeriodDropBeforeRule.java | 15 + .../coordinator/rules/PeriodDropRule.java | 15 + .../coordinator/rules/PeriodLoadRule.java | 19 +- .../druid/server/coordinator/rules/Rule.java | 2 + .../druid/server/coordinator/rules/Rules.java | 3 + .../druid/server/http/RulesResource.java | 38 +++ .../druid/server/http/RulesResourceTest.java | 287 ++++++++++++++++++ 14 files changed, 474 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverBroadcastDistributionRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverBroadcastDistributionRule.java index ef5094cbea4a..f770c686df67 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverBroadcastDistributionRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverBroadcastDistributionRule.java @@ -56,6 +56,12 @@ public boolean appliesTo(Interval interval, DateTime referenceTimestamp) return true; } + @Override + public Interval getInterval(DateTime referenceTimestamp) + { + return Rules.FOREVER_INTERVAL; + } + @Override public boolean equals(Object o) { @@ -75,4 +81,10 @@ public int hashCode() { return Objects.hash(getType()); } + + @Override + public String toString() + { + return "ForeverBroadcastDistributionRule{}"; + } } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverDropRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverDropRule.java index 19479c015b91..b9c7352539ef 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverDropRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverDropRule.java @@ -46,4 +46,16 @@ public boolean appliesTo(Interval interval, DateTime referenceTimestamp) { return true; } + + @Override + public Interval getInterval(DateTime referenceTimestamp) + { + return Rules.FOREVER_INTERVAL; + } + + @Override + public String toString() + { + return "ForeverDropRule{}"; + } } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverLoadRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverLoadRule.java index 35f22fa555f8..5166ed736d2c 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverLoadRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverLoadRule.java @@ -60,4 +60,18 @@ public boolean appliesTo(Interval interval, DateTime referenceTimestamp) return true; } + @Override + public Interval getInterval(DateTime referenceTimestamp) + { + return Rules.FOREVER_INTERVAL; + } + + @Override + public String toString() + { + return "ForeverLoadRule{" + + "tieredReplicants=" + getTieredReplicants() + + ", useDefaultTierForNull=" + useDefaultTierForNull() + + '}'; + } } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalBroadcastDistributionRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalBroadcastDistributionRule.java index b1bf29eedd20..e0db2a01e504 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalBroadcastDistributionRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalBroadcastDistributionRule.java @@ -65,6 +65,12 @@ public Interval getInterval() return interval; } + @Override + public Interval getInterval(DateTime referenceTimestamp) + { + return interval; + } + @Override public boolean equals(Object o) { @@ -83,4 +89,12 @@ public int hashCode() { return Objects.hash(getInterval()); } + + @Override + public String toString() + { + return "IntervalBroadcastDistributionRule{" + + "interval=" + interval + + '}'; + } } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalDropRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalDropRule.java index b2959e925d28..e53c06c8ea57 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalDropRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalDropRule.java @@ -66,6 +66,12 @@ public boolean appliesTo(Interval theInterval, DateTime referenceTimestamp) return interval.contains(theInterval); } + @Override + public Interval getInterval(DateTime referenceTimestamp) + { + return interval; + } + @Override public boolean equals(Object o) { @@ -84,4 +90,12 @@ public int hashCode() { return Objects.hash(interval); } + + @Override + public String toString() + { + return "IntervalDropRule{" + + "interval=" + interval + + '}'; + } } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalLoadRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalLoadRule.java index 209f5c24d1e4..a5fb46503f58 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalLoadRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalLoadRule.java @@ -21,7 +21,6 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; -import org.apache.druid.java.util.common.logger.Logger; import org.apache.druid.timeline.DataSegment; import org.joda.time.DateTime; import org.joda.time.Interval; @@ -34,8 +33,6 @@ */ public class IntervalLoadRule extends LoadRule { - private static final Logger log = new Logger(IntervalLoadRule.class); - private final Interval interval; @JsonCreator @@ -74,6 +71,12 @@ public boolean appliesTo(Interval theInterval, DateTime referenceTimestamp) return Rules.eligibleForLoad(interval, theInterval); } + @Override + public Interval getInterval(DateTime referenceTimestamp) + { + return interval; + } + @Override public boolean equals(Object o) { @@ -95,4 +98,14 @@ public int hashCode() { return Objects.hash(super.hashCode(), interval); } + + @Override + public String toString() + { + return "IntervalLoadRule{" + + "interval=" + interval + + ", tieredReplicants=" + getTieredReplicants() + + ", useDefaultTierForNull=" + useDefaultTierForNull() + + '}'; + } } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodBroadcastDistributionRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodBroadcastDistributionRule.java index d48353d3e50a..a88e75fdb4e4 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodBroadcastDistributionRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodBroadcastDistributionRule.java @@ -65,6 +65,12 @@ public boolean appliesTo(Interval interval, DateTime referenceTimestamp) return Rules.eligibleForLoad(period, interval, referenceTimestamp, includeFuture); } + @Override + public Interval getInterval(DateTime referenceTimestamp) + { + return new Interval(referenceTimestamp.minus(period), referenceTimestamp); + } + @JsonProperty public Period getPeriod() { @@ -96,4 +102,13 @@ public int hashCode() { return Objects.hash(getPeriod(), isIncludeFuture()); } + + @Override + public String toString() + { + return "PeriodBroadcastDistributionRule{" + + "period=" + period + + ", includeFuture=" + includeFuture + + '}'; + } } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropBeforeRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropBeforeRule.java index 6654b385eac3..262d00a7aae0 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropBeforeRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropBeforeRule.java @@ -21,6 +21,7 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.druid.java.util.common.DateTimes; import org.apache.druid.timeline.DataSegment; import org.joda.time.DateTime; import org.joda.time.Interval; @@ -63,4 +64,18 @@ public boolean appliesTo(Interval theInterval, DateTime referenceTimestamp) final DateTime periodAgo = referenceTimestamp.minus(period); return theInterval.getEndMillis() <= periodAgo.getMillis(); } + + @Override + public Interval getInterval(DateTime referenceTimestamp) + { + return new Interval(DateTimes.utc(Long.MIN_VALUE), referenceTimestamp.minus(period)); + } + + @Override + public String toString() + { + return "PeriodDropBeforeRule{" + + "period=" + period + + '}'; + } } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropRule.java index da17b4c2a9d9..a31ee254b676 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropRule.java @@ -80,4 +80,19 @@ public boolean appliesTo(Interval theInterval, DateTime referenceTimestamp) return currInterval.contains(theInterval); } } + + @Override + public Interval getInterval(DateTime referenceTimestamp) + { + return new Interval(referenceTimestamp.minus(period), referenceTimestamp); + } + + @Override + public String toString() + { + return "PeriodDropRule{" + + "period=" + period + + ", includeFuture=" + includeFuture + + '}'; + } } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java index 1d2b4e187716..d7b814c66beb 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java @@ -21,7 +21,6 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; -import org.apache.druid.java.util.common.logger.Logger; import org.apache.druid.timeline.DataSegment; import org.joda.time.DateTime; import org.joda.time.Interval; @@ -35,7 +34,6 @@ */ public class PeriodLoadRule extends LoadRule { - private static final Logger log = new Logger(PeriodLoadRule.class); static final boolean DEFAULT_INCLUDE_FUTURE = true; private final Period period; @@ -85,6 +83,12 @@ public boolean appliesTo(Interval interval, DateTime referenceTimestamp) return Rules.eligibleForLoad(period, interval, referenceTimestamp, includeFuture); } + @Override + public Interval getInterval(DateTime referenceTimestamp) + { + return new Interval(referenceTimestamp.minus(period), referenceTimestamp); + } + @Override public boolean equals(Object o) { @@ -106,4 +110,15 @@ public int hashCode() { return Objects.hash(super.hashCode(), period, includeFuture); } + + @Override + public String toString() + { + return "PeriodLoadRule{" + + "period=" + period + + ", includeFuture=" + includeFuture + + ", tieredReplicants=" + getTieredReplicants() + + ", useDefaultTierForNull=" + useDefaultTierForNull() + + '}'; + } } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/Rule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/Rule.java index a66101d0fe14..0f1f821f8fc5 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/Rule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/Rule.java @@ -50,4 +50,6 @@ public interface Rule boolean appliesTo(Interval interval, DateTime referenceTimestamp); void run(DataSegment segment, SegmentActionHandler segmentHandler); + + Interval getInterval(DateTime referenceTimestamp); } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java index 9cb6094886c7..b8d63ec8a078 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java @@ -19,12 +19,15 @@ package org.apache.druid.server.coordinator.rules; +import org.apache.druid.java.util.common.DateTimes; import org.joda.time.DateTime; import org.joda.time.Interval; import org.joda.time.Period; public class Rules { + public static final Interval FOREVER_INTERVAL = new Interval(DateTimes.utc(Long.MIN_VALUE), DateTimes.utc(Long.MAX_VALUE)); + public static boolean eligibleForLoad(Interval src, Interval target) { return src.overlaps(target); diff --git a/server/src/main/java/org/apache/druid/server/http/RulesResource.java b/server/src/main/java/org/apache/druid/server/http/RulesResource.java index beb223a6cc65..3e5b5d78f38b 100644 --- a/server/src/main/java/org/apache/druid/server/http/RulesResource.java +++ b/server/src/main/java/org/apache/druid/server/http/RulesResource.java @@ -25,11 +25,15 @@ import org.apache.druid.audit.AuditEntry; import org.apache.druid.audit.AuditInfo; import org.apache.druid.audit.AuditManager; +import org.apache.druid.error.InvalidInput; +import org.apache.druid.java.util.common.DateTimes; import org.apache.druid.java.util.common.Intervals; import org.apache.druid.metadata.MetadataRuleManager; import org.apache.druid.server.coordinator.rules.Rule; +import org.apache.druid.server.coordinator.rules.Rules; import org.apache.druid.server.http.security.RulesResourceFilter; import org.apache.druid.server.http.security.StateResourceFilter; +import org.joda.time.DateTime; import org.joda.time.Interval; import javax.servlet.http.HttpServletRequest; @@ -108,6 +112,7 @@ public Response setDatasourceRules( ) { try { + validateRules(rules); final AuditInfo auditInfo = new AuditInfo(author, comment, req.getRemoteAddr()); if (databaseRuleManager.overrideRule(dataSourceName, rules, auditInfo)) { return Response.ok().build(); @@ -181,4 +186,37 @@ private List getRuleHistory( return auditManager.fetchAuditHistory(AUDIT_HISTORY_TYPE, theInterval); } + /** + * Validate rules. Throws an exception if a rule contain an interval that will overshadow another rules' interval. + * Rules that will be evaluated at some point are considered to be non-overshadowing. + * @param rules Datasource rules. + */ + private void validateRules(final List 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.getInterval(now); + final Interval nextInterval = nextRule.getInterval(now); + if (currInterval.contains(nextInterval)) { + // If the current rule overshaows the next rule even at the intervals' boundaries, then we know that the next + // rule will always be a no-op. Also, a forever rule spans eternity and overshadows everything that follows it. + if (Rules.FOREVER_INTERVAL.equals(currInterval) || + (currRule.getInterval(currInterval.getStart()).contains(nextRule.getInterval(currInterval.getStart())) + && currRule.getInterval(currInterval.getEnd()).contains(nextRule.getInterval(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 + ); + } + } + } + } } diff --git a/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java b/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java index 5b52e3d30ee9..f3b3af68ffa6 100644 --- a/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java +++ b/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java @@ -23,16 +23,31 @@ import org.apache.druid.audit.AuditEntry; import org.apache.druid.audit.AuditInfo; import org.apache.druid.audit.AuditManager; +import org.apache.druid.error.DruidExceptionMatcher; import org.apache.druid.java.util.common.DateTimes; import org.apache.druid.java.util.common.Intervals; +import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.metadata.MetadataRuleManager; +import org.apache.druid.server.coordinator.rules.ForeverBroadcastDistributionRule; +import org.apache.druid.server.coordinator.rules.ForeverDropRule; +import org.apache.druid.server.coordinator.rules.ForeverLoadRule; +import org.apache.druid.server.coordinator.rules.IntervalBroadcastDistributionRule; +import org.apache.druid.server.coordinator.rules.IntervalLoadRule; +import org.apache.druid.server.coordinator.rules.PeriodBroadcastDistributionRule; +import org.apache.druid.server.coordinator.rules.PeriodDropBeforeRule; +import org.apache.druid.server.coordinator.rules.PeriodDropRule; +import org.apache.druid.server.coordinator.rules.PeriodLoadRule; +import org.apache.druid.server.coordinator.rules.Rule; import org.easymock.EasyMock; import org.joda.time.Interval; +import org.joda.time.Period; import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import javax.servlet.http.HttpServletRequest; import javax.ws.rs.core.Response; +import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -151,6 +166,278 @@ public void testGetDatasourceRuleHistoryWithWrongCount() EasyMock.verify(auditManager); } + @Test + public void testSetDatasourceRulesWithEffectivelyNoRule() + { + EasyMock.expect(databaseRuleManager.overrideRule(EasyMock.anyObject(), EasyMock.anyObject(), EasyMock.anyObject())) + .andReturn(true).times(2); + EasyMock.replay(databaseRuleManager); + + final RulesResource rulesResource = new RulesResource(databaseRuleManager, auditManager); + final Response resp1 = rulesResource.setDatasourceRules("dataSource1", null, null, null, EasyMock.createMock(HttpServletRequest.class)); + Assert.assertEquals(200, resp1.getStatus()); + + final Response resp2 = rulesResource.setDatasourceRules("dataSource1", new ArrayList<>(), null, null, EasyMock.createMock(HttpServletRequest.class)); + Assert.assertEquals(200, resp2.getStatus()); + EasyMock.verify(databaseRuleManager); + } + + @Test + public void testSetDatasourceRulesWithInvalidLoadRules() + { + EasyMock.replay(auditManager); + + final RulesResource rulesResource = new RulesResource(databaseRuleManager, auditManager); + final HttpServletRequest req = EasyMock.createMock(HttpServletRequest.class); + + final IntervalLoadRule loadInterval1 = new IntervalLoadRule( + Intervals.of("2023-07-29T01:00:00Z/2023-12-30T01:00:00Z"), + null, + null + ); + final IntervalLoadRule loadInterval2 = new IntervalLoadRule( + Intervals.of("2023-09-29T01:00:00Z/2023-10-30T01:00:00Z"), + null, + null + ); + final PeriodLoadRule loadPT1H = new PeriodLoadRule(new Period("PT1H"), null, null, null); + final PeriodLoadRule loadP3M = new PeriodLoadRule(new Period("P3M"), null, null, null); + final PeriodLoadRule loadP6M = new PeriodLoadRule(new Period("P6M"), null, null, null); + final ForeverLoadRule loadForever = new ForeverLoadRule(null, null); + + final List rules = new ArrayList<>(); + rules.add(loadP6M); + rules.add(loadP3M); + rules.add(loadForever); + + DruidExceptionMatcher.invalidInput().expectMessageContains( + StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", loadP6M, loadP3M) + ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + + rules.clear(); + rules.add(loadP3M); + rules.add(loadForever); + rules.add(loadP6M); + + DruidExceptionMatcher.invalidInput().expectMessageContains( + StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", loadForever, loadP6M) + ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + + rules.clear(); + rules.add(loadForever); + rules.add(loadPT1H); + rules.add(loadInterval2); + rules.add(loadP6M); + + DruidExceptionMatcher.invalidInput().expectMessageContains( + StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", loadForever, loadPT1H) + ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + + rules.clear(); + rules.add(loadInterval1); + rules.add(loadInterval2); + rules.add(loadP6M); + rules.add(loadForever); + + DruidExceptionMatcher.invalidInput().expectMessageContains( + StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", loadInterval1, loadInterval2) + ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + + rules.clear(); + rules.add(loadP6M); + rules.add(loadInterval1); + rules.add(loadForever); + rules.add(loadInterval2); + + DruidExceptionMatcher.invalidInput().expectMessageContains( + StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", loadForever, loadInterval2) + ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + } + + @Test + public void testDatasourceRulesWithInvalidDropRules() + { + EasyMock.replay(auditManager); + + final RulesResource rulesResource = new RulesResource(databaseRuleManager, auditManager); + final HttpServletRequest req = EasyMock.createMock(HttpServletRequest.class); + + final PeriodDropBeforeRule dropBeforeP3M = new PeriodDropBeforeRule(new Period("P3M")); + final PeriodDropBeforeRule dropBeforeP6M = new PeriodDropBeforeRule(new Period("P6M")); + final PeriodDropRule dropByP1M = new PeriodDropRule(new Period("P1M"), true); + final PeriodDropRule dropByP2M = new PeriodDropRule(new Period("P2M"), true); + final ForeverDropRule dropForever = new ForeverDropRule(); + + final List rules = new ArrayList<>(); + rules.add(dropBeforeP3M); + rules.add(dropBeforeP6M); + rules.add(dropByP1M); + rules.add(dropForever); + + DruidExceptionMatcher.invalidInput().expectMessageContains( + StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", dropBeforeP3M, dropBeforeP6M) + ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + + rules.clear(); + rules.add(dropByP2M); + rules.add(dropByP1M); + rules.add(dropForever); + + DruidExceptionMatcher.invalidInput().expectMessageContains( + StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", dropByP2M, dropByP1M) + ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + + rules.clear(); + rules.add(dropForever); + rules.add(dropByP1M); + rules.add(dropByP2M); + + DruidExceptionMatcher.invalidInput().expectMessageContains( + StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", dropForever, dropByP1M) + ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + } + + @Test + public void testDatasourceRulesWithInvalidBroadcastRules() + { + EasyMock.replay(auditManager); + + final RulesResource rulesResource = new RulesResource(databaseRuleManager, auditManager); + final HttpServletRequest req = EasyMock.createMock(HttpServletRequest.class); + + final ForeverBroadcastDistributionRule broadcastForever = new ForeverBroadcastDistributionRule(); + final PeriodBroadcastDistributionRule broadcastPeriodPT1H = new PeriodBroadcastDistributionRule(new Period("PT1H"), true); + final PeriodBroadcastDistributionRule broadcastPeriodPT2H = new PeriodBroadcastDistributionRule(new Period("PT2H"), true); + final IntervalBroadcastDistributionRule broadcastInterval1 = new IntervalBroadcastDistributionRule( + Intervals.of("2000-09-29T01:00:00Z/2050-10-30T01:00:00Z") + ); + final IntervalBroadcastDistributionRule broadcastInterval2 = new IntervalBroadcastDistributionRule( + Intervals.of("2010-09-29T01:00:00Z/2020-10-30T01:00:00Z") + ); + + final List rules = new ArrayList<>(); + rules.add(broadcastInterval1); + rules.add(broadcastInterval2); + rules.add(broadcastForever); + + DruidExceptionMatcher.invalidInput().expectMessageContains( + StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", broadcastInterval1, broadcastInterval2) + ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + + rules.clear(); + rules.add(broadcastPeriodPT2H); + rules.add(broadcastPeriodPT1H); + rules.add(broadcastForever); + + DruidExceptionMatcher.invalidInput().expectMessageContains( + StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", broadcastPeriodPT2H, broadcastPeriodPT1H) + ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + + + rules.clear(); + rules.add(broadcastPeriodPT1H); + rules.add(broadcastPeriodPT2H); + rules.add(broadcastInterval2); + rules.add(broadcastForever); + rules.add(broadcastInterval1); + + DruidExceptionMatcher.invalidInput().expectMessageContains( + StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", broadcastForever, broadcastInterval1) + ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + } + + @Test + public void testSetDatasourceRulesWithDifferentInvalidRules() + { + EasyMock.replay(auditManager); + + final RulesResource rulesResource = new RulesResource(databaseRuleManager, auditManager); + final HttpServletRequest req = EasyMock.createMock(HttpServletRequest.class); + + final IntervalLoadRule loadLargeInteval = new IntervalLoadRule( + Intervals.of("1980-07-29T01:00:00Z/2050-12-30T01:00:00Z"), + null, + null + ); + final IntervalLoadRule loadSmallInterval = new IntervalLoadRule( + Intervals.of("2020-09-29T01:00:00Z/2025-10-30T01:00:00Z"), + null, + null + ); + final PeriodLoadRule periodPT1H = new PeriodLoadRule(new Period("PT1H"), null, null, null); + final PeriodLoadRule periodP3M = new PeriodLoadRule(new Period("P3M"), null, null, null); + final PeriodLoadRule periodP6M = new PeriodLoadRule(new Period("P6M"), null, null, null); + final ForeverLoadRule foreverLoadRule = new ForeverLoadRule(null, null); + final ForeverDropRule foreverDropRule = new ForeverDropRule(); + final PeriodBroadcastDistributionRule broadcastPeriodPT15m = new PeriodBroadcastDistributionRule(new Period("PT15m"), true); + + final List rules = new ArrayList<>(); + rules.add(loadSmallInterval); + rules.add(loadLargeInteval); + rules.add(broadcastPeriodPT15m); + rules.add(periodPT1H); + rules.add(periodP3M); + rules.add(periodP6M); + rules.add(foreverLoadRule); + rules.add(foreverDropRule); + + DruidExceptionMatcher.invalidInput().expectMessageContains( + StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", foreverLoadRule, foreverDropRule) + ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + } + + @Test + public void testSetDatasourceRulesWithValidRules() + { + EasyMock.expect(databaseRuleManager.overrideRule(EasyMock.anyObject(), EasyMock.anyObject(), EasyMock.anyObject())) + .andReturn(true).anyTimes(); + EasyMock.replay(databaseRuleManager); + final RulesResource rulesResource = new RulesResource(databaseRuleManager, auditManager); + + final IntervalLoadRule loadLargeInteval = new IntervalLoadRule( + Intervals.of("1980-07-29T01:00:00Z/2050-12-30T01:00:00Z"), + null, + null + ); + final IntervalLoadRule loadSmallInterval = new IntervalLoadRule( + Intervals.of("2020-09-29T01:00:00Z/2025-10-30T01:00:00Z"), + null, + null + ); + final PeriodLoadRule periodPT1H = new PeriodLoadRule(new Period("PT1H"), null, null, null); + final PeriodLoadRule periodP3M = new PeriodLoadRule(new Period("P3M"), null, null, null); + final PeriodLoadRule periodP6M = new PeriodLoadRule(new Period("P6M"), null, null, null); + final ForeverLoadRule foreverLoadRule = new ForeverLoadRule(null, null); + final ForeverDropRule foreverDropRule = new ForeverDropRule(); + final PeriodBroadcastDistributionRule broadcastPeriodPT15m = new PeriodBroadcastDistributionRule(new Period("PT15m"), true); + + final List rules = new ArrayList<>(); + rules.add(loadSmallInterval); + rules.add(loadLargeInteval); + rules.add(broadcastPeriodPT15m); + rules.add(periodPT1H); + rules.add(periodP3M); + rules.add(periodP6M); + rules.add(foreverLoadRule); + + final Response resp = rulesResource.setDatasourceRules("dataSource1", rules, null, null, EasyMock.createMock(HttpServletRequest.class)); + Assert.assertEquals(200, resp.getStatus()); + EasyMock.verify(databaseRuleManager); + + rules.clear(); + rules.add(broadcastPeriodPT15m); + rules.add(periodPT1H); + rules.add(periodP3M); + rules.add(periodP6M); + rules.add(loadSmallInterval); + rules.add(loadLargeInteval); + rules.add(foreverDropRule); + + final Response resp2 = rulesResource.setDatasourceRules("dataSource1", rules, null, null, EasyMock.createMock(HttpServletRequest.class)); + Assert.assertEquals(200, resp2.getStatus()); + EasyMock.verify(databaseRuleManager); + } + @Test public void testGetAllDatasourcesRuleHistoryWithCount() { From 91d4797cdfe9c4ef117756ef0c07f5f1245e97cf Mon Sep 17 00:00:00 2001 From: Abhishek Balaji Radhakrishnan Date: Tue, 19 Sep 2023 21:20:58 -0700 Subject: [PATCH 2/9] Address review comments. --- .../ForeverBroadcastDistributionRule.java | 5 ++- .../coordinator/rules/ForeverDropRule.java | 5 ++- .../coordinator/rules/ForeverLoadRule.java | 5 ++- .../IntervalBroadcastDistributionRule.java | 2 +- .../coordinator/rules/IntervalDropRule.java | 2 +- .../coordinator/rules/IntervalLoadRule.java | 2 +- .../PeriodBroadcastDistributionRule.java | 4 +- .../rules/PeriodDropBeforeRule.java | 2 +- .../coordinator/rules/PeriodDropRule.java | 2 +- .../coordinator/rules/PeriodLoadRule.java | 2 +- .../druid/server/coordinator/rules/Rule.java | 8 +++- .../druid/server/coordinator/rules/Rules.java | 42 ++++++++++++++++++- .../druid/server/http/RulesResource.java | 39 +---------------- .../druid/server/http/RulesResourceTest.java | 10 ++++- 14 files changed, 74 insertions(+), 56 deletions(-) diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverBroadcastDistributionRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverBroadcastDistributionRule.java index f770c686df67..3fc808450ebf 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverBroadcastDistributionRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverBroadcastDistributionRule.java @@ -21,6 +21,7 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.druid.java.util.common.Intervals; import org.apache.druid.timeline.DataSegment; import org.joda.time.DateTime; import org.joda.time.Interval; @@ -57,9 +58,9 @@ public boolean appliesTo(Interval interval, DateTime referenceTimestamp) } @Override - public Interval getInterval(DateTime referenceTimestamp) + public Interval getEligibleInterval(DateTime referenceTimestamp) { - return Rules.FOREVER_INTERVAL; + return Intervals.ETERNITY; } @Override diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverDropRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverDropRule.java index b9c7352539ef..7c5b54bf8c2d 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverDropRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverDropRule.java @@ -20,6 +20,7 @@ package org.apache.druid.server.coordinator.rules; import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.druid.java.util.common.Intervals; import org.apache.druid.timeline.DataSegment; import org.joda.time.DateTime; import org.joda.time.Interval; @@ -48,9 +49,9 @@ public boolean appliesTo(Interval interval, DateTime referenceTimestamp) } @Override - public Interval getInterval(DateTime referenceTimestamp) + public Interval getEligibleInterval(DateTime referenceTimestamp) { - return Rules.FOREVER_INTERVAL; + return Intervals.ETERNITY; } @Override diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverLoadRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverLoadRule.java index 5166ed736d2c..55a9a992a00f 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverLoadRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverLoadRule.java @@ -21,6 +21,7 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.druid.java.util.common.Intervals; import org.apache.druid.timeline.DataSegment; import org.joda.time.DateTime; import org.joda.time.Interval; @@ -61,9 +62,9 @@ public boolean appliesTo(Interval interval, DateTime referenceTimestamp) } @Override - public Interval getInterval(DateTime referenceTimestamp) + public Interval getEligibleInterval(DateTime referenceTimestamp) { - return Rules.FOREVER_INTERVAL; + return Intervals.ETERNITY; } @Override diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalBroadcastDistributionRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalBroadcastDistributionRule.java index e0db2a01e504..96e7123630ad 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalBroadcastDistributionRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalBroadcastDistributionRule.java @@ -66,7 +66,7 @@ public Interval getInterval() } @Override - public Interval getInterval(DateTime referenceTimestamp) + public Interval getEligibleInterval(DateTime referenceTimestamp) { return interval; } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalDropRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalDropRule.java index e53c06c8ea57..6ed11d49bf60 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalDropRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalDropRule.java @@ -67,7 +67,7 @@ public boolean appliesTo(Interval theInterval, DateTime referenceTimestamp) } @Override - public Interval getInterval(DateTime referenceTimestamp) + public Interval getEligibleInterval(DateTime referenceTimestamp) { return interval; } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalLoadRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalLoadRule.java index a5fb46503f58..c66675c75fa4 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalLoadRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalLoadRule.java @@ -72,7 +72,7 @@ public boolean appliesTo(Interval theInterval, DateTime referenceTimestamp) } @Override - public Interval getInterval(DateTime referenceTimestamp) + public Interval getEligibleInterval(DateTime referenceTimestamp) { return interval; } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodBroadcastDistributionRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodBroadcastDistributionRule.java index a88e75fdb4e4..8406776dcc13 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodBroadcastDistributionRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodBroadcastDistributionRule.java @@ -66,9 +66,9 @@ public boolean appliesTo(Interval interval, DateTime referenceTimestamp) } @Override - public Interval getInterval(DateTime referenceTimestamp) + public Interval getEligibleInterval(DateTime referenceTimestamp) { - return new Interval(referenceTimestamp.minus(period), referenceTimestamp); + return new Interval(period, referenceTimestamp); } @JsonProperty diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropBeforeRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropBeforeRule.java index 262d00a7aae0..07e220a1dfdc 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropBeforeRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropBeforeRule.java @@ -66,7 +66,7 @@ public boolean appliesTo(Interval theInterval, DateTime referenceTimestamp) } @Override - public Interval getInterval(DateTime referenceTimestamp) + public Interval getEligibleInterval(DateTime referenceTimestamp) { return new Interval(DateTimes.utc(Long.MIN_VALUE), referenceTimestamp.minus(period)); } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropRule.java index a31ee254b676..63c519250245 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropRule.java @@ -82,7 +82,7 @@ public boolean appliesTo(Interval theInterval, DateTime referenceTimestamp) } @Override - public Interval getInterval(DateTime referenceTimestamp) + public Interval getEligibleInterval(DateTime referenceTimestamp) { return new Interval(referenceTimestamp.minus(period), referenceTimestamp); } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java index d7b814c66beb..8834c7c412ae 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java @@ -84,7 +84,7 @@ public boolean appliesTo(Interval interval, DateTime referenceTimestamp) } @Override - public Interval getInterval(DateTime referenceTimestamp) + public Interval getEligibleInterval(DateTime referenceTimestamp) { return new Interval(referenceTimestamp.minus(period), referenceTimestamp); } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/Rule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/Rule.java index 0f1f821f8fc5..2faa248a2be4 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/Rule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/Rule.java @@ -51,5 +51,11 @@ public interface Rule void run(DataSegment segment, SegmentActionHandler segmentHandler); - Interval getInterval(DateTime referenceTimestamp); + /** + * Return an eligible interval from the reference timestamp. Implemntations + * must return a valid interval based on the rule type. + * @param referenceTimestamp base timestamp + * @return + */ + Interval getEligibleInterval(DateTime referenceTimestamp); } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java index b8d63ec8a078..a25a1b363f4a 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java @@ -19,15 +19,17 @@ package org.apache.druid.server.coordinator.rules; +import org.apache.druid.error.InvalidInput; import org.apache.druid.java.util.common.DateTimes; +import org.apache.druid.java.util.common.Intervals; import org.joda.time.DateTime; import org.joda.time.Interval; import org.joda.time.Period; +import java.util.List; + public class Rules { - public static final Interval FOREVER_INTERVAL = new Interval(DateTimes.utc(Long.MIN_VALUE), DateTimes.utc(Long.MAX_VALUE)); - public static boolean eligibleForLoad(Interval src, Interval target) { return src.overlaps(target); @@ -46,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 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 + ); + } + } + } + } } diff --git a/server/src/main/java/org/apache/druid/server/http/RulesResource.java b/server/src/main/java/org/apache/druid/server/http/RulesResource.java index 3e5b5d78f38b..99d7d5d1caf4 100644 --- a/server/src/main/java/org/apache/druid/server/http/RulesResource.java +++ b/server/src/main/java/org/apache/druid/server/http/RulesResource.java @@ -25,15 +25,12 @@ import org.apache.druid.audit.AuditEntry; import org.apache.druid.audit.AuditInfo; import org.apache.druid.audit.AuditManager; -import org.apache.druid.error.InvalidInput; -import org.apache.druid.java.util.common.DateTimes; import org.apache.druid.java.util.common.Intervals; import org.apache.druid.metadata.MetadataRuleManager; import org.apache.druid.server.coordinator.rules.Rule; import org.apache.druid.server.coordinator.rules.Rules; import org.apache.druid.server.http.security.RulesResourceFilter; import org.apache.druid.server.http.security.StateResourceFilter; -import org.joda.time.DateTime; import org.joda.time.Interval; import javax.servlet.http.HttpServletRequest; @@ -112,7 +109,7 @@ public Response setDatasourceRules( ) { try { - validateRules(rules); + Rules.validateRules(rules); final AuditInfo auditInfo = new AuditInfo(author, comment, req.getRemoteAddr()); if (databaseRuleManager.overrideRule(dataSourceName, rules, auditInfo)) { return Response.ok().build(); @@ -185,38 +182,4 @@ private List getRuleHistory( } return auditManager.fetchAuditHistory(AUDIT_HISTORY_TYPE, theInterval); } - - /** - * Validate rules. Throws an exception if a rule contain an interval that will overshadow another rules' interval. - * Rules that will be evaluated at some point are considered to be non-overshadowing. - * @param rules Datasource rules. - */ - private void validateRules(final List 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.getInterval(now); - final Interval nextInterval = nextRule.getInterval(now); - if (currInterval.contains(nextInterval)) { - // If the current rule overshaows the next rule even at the intervals' boundaries, then we know that the next - // rule will always be a no-op. Also, a forever rule spans eternity and overshadows everything that follows it. - if (Rules.FOREVER_INTERVAL.equals(currInterval) || - (currRule.getInterval(currInterval.getStart()).contains(nextRule.getInterval(currInterval.getStart())) - && currRule.getInterval(currInterval.getEnd()).contains(nextRule.getInterval(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 - ); - } - } - } - } } diff --git a/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java b/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java index f3b3af68ffa6..92ea60221de3 100644 --- a/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java +++ b/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java @@ -233,6 +233,15 @@ public void testSetDatasourceRulesWithInvalidLoadRules() StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", loadForever, loadPT1H) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + rules.clear(); + rules.add(loadPT1H); + rules.add(loadPT1H); + rules.add(loadP6M); + + DruidExceptionMatcher.invalidInput().expectMessageContains( + StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", loadPT1H, loadPT1H) + ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + rules.clear(); rules.add(loadInterval1); rules.add(loadInterval2); @@ -333,7 +342,6 @@ public void testDatasourceRulesWithInvalidBroadcastRules() StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", broadcastPeriodPT2H, broadcastPeriodPT1H) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); - rules.clear(); rules.add(broadcastPeriodPT1H); rules.add(broadcastPeriodPT2H); From 2166d6c4abc098186edc1f8d611c8525aa57de82 Mon Sep 17 00:00:00 2001 From: Abhishek Radhakrishnan Date: Tue, 19 Sep 2023 22:48:06 -0700 Subject: [PATCH 3/9] Apply suggestions from code review Co-authored-by: Laksh Singla --- .../java/org/apache/druid/server/coordinator/rules/Rule.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/Rule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/Rule.java index 2faa248a2be4..c34b086484c8 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/Rule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/Rule.java @@ -52,10 +52,9 @@ public interface Rule void run(DataSegment segment, SegmentActionHandler segmentHandler); /** - * Return an eligible interval from the reference timestamp. Implemntations + * Return an eligible interval from the reference timestamp. Implementations * must return a valid interval based on the rule type. * @param referenceTimestamp base timestamp - * @return */ Interval getEligibleInterval(DateTime referenceTimestamp); } From c3edb12849a4c65b7098cefcfef449e9228c13ae Mon Sep 17 00:00:00 2001 From: Abhishek Balaji Radhakrishnan Date: Tue, 19 Sep 2023 23:15:35 -0700 Subject: [PATCH 4/9] includeFuture change and unit test. --- .../PeriodBroadcastDistributionRule.java | 3 +- .../coordinator/rules/PeriodDropRule.java | 4 +- .../coordinator/rules/PeriodLoadRule.java | 3 +- .../druid/server/http/RulesResourceTest.java | 64 +++++++++++++++++-- 4 files changed, 65 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodBroadcastDistributionRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodBroadcastDistributionRule.java index 8406776dcc13..5de02add3316 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodBroadcastDistributionRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodBroadcastDistributionRule.java @@ -68,7 +68,8 @@ public boolean appliesTo(Interval interval, DateTime referenceTimestamp) @Override public Interval getEligibleInterval(DateTime referenceTimestamp) { - return new Interval(period, referenceTimestamp); + return includeFuture ? new Interval(referenceTimestamp.minus(period), referenceTimestamp.plus(period)) + : new Interval(referenceTimestamp.minus(period), referenceTimestamp); } @JsonProperty diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropRule.java index 63c519250245..c999bb01ad96 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropRule.java @@ -27,6 +27,7 @@ import org.joda.time.Period; /** + * */ public class PeriodDropRule extends DropRule { @@ -84,7 +85,8 @@ public boolean appliesTo(Interval theInterval, DateTime referenceTimestamp) @Override public Interval getEligibleInterval(DateTime referenceTimestamp) { - return new Interval(referenceTimestamp.minus(period), referenceTimestamp); + return includeFuture ? new Interval(referenceTimestamp.minus(period), referenceTimestamp.plus(period)) + : new Interval(referenceTimestamp.minus(period), referenceTimestamp); } @Override diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java index 8834c7c412ae..0481195ec0a7 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java @@ -86,7 +86,8 @@ public boolean appliesTo(Interval interval, DateTime referenceTimestamp) @Override public Interval getEligibleInterval(DateTime referenceTimestamp) { - return new Interval(referenceTimestamp.minus(period), referenceTimestamp); + return includeFuture ? new Interval(referenceTimestamp.minus(period), referenceTimestamp.plus(period)) + : new Interval(referenceTimestamp.minus(period), referenceTimestamp); } @Override diff --git a/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java b/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java index 92ea60221de3..909557c73f9a 100644 --- a/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java +++ b/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java @@ -200,9 +200,10 @@ public void testSetDatasourceRulesWithInvalidLoadRules() null, null ); - final PeriodLoadRule loadPT1H = new PeriodLoadRule(new Period("PT1H"), null, null, null); - final PeriodLoadRule loadP3M = new PeriodLoadRule(new Period("P3M"), null, null, null); - final PeriodLoadRule loadP6M = new PeriodLoadRule(new Period("P6M"), null, null, null); + final PeriodLoadRule loadPT1H = new PeriodLoadRule(new Period("PT1H"), true, null, null); + final PeriodLoadRule loadPT1HNoFuture = new PeriodLoadRule(new Period("PT1H"), false, null, null); + final PeriodLoadRule loadP3M = new PeriodLoadRule(new Period("P3M"), true, null, null); + final PeriodLoadRule loadP6M = new PeriodLoadRule(new Period("P6M"), true, null, null); final ForeverLoadRule loadForever = new ForeverLoadRule(null, null); final List rules = new ArrayList<>(); @@ -233,6 +234,15 @@ public void testSetDatasourceRulesWithInvalidLoadRules() StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", loadForever, loadPT1H) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + rules.clear(); + rules.add(loadPT1H); + rules.add(loadPT1HNoFuture); + rules.add(loadP6M); + + DruidExceptionMatcher.invalidInput().expectMessageContains( + StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", loadPT1H, loadPT1HNoFuture) + ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + rules.clear(); rules.add(loadPT1H); rules.add(loadPT1H); @@ -274,6 +284,7 @@ public void testDatasourceRulesWithInvalidDropRules() final PeriodDropBeforeRule dropBeforeP3M = new PeriodDropBeforeRule(new Period("P3M")); final PeriodDropBeforeRule dropBeforeP6M = new PeriodDropBeforeRule(new Period("P6M")); final PeriodDropRule dropByP1M = new PeriodDropRule(new Period("P1M"), true); + final PeriodDropRule dropByP1MNoFuture = new PeriodDropRule(new Period("P1M"), false); final PeriodDropRule dropByP2M = new PeriodDropRule(new Period("P2M"), true); final ForeverDropRule dropForever = new ForeverDropRule(); @@ -296,6 +307,16 @@ public void testDatasourceRulesWithInvalidDropRules() StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", dropByP2M, dropByP1M) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + rules.clear(); + rules.add(dropByP1M); + rules.add(dropByP1MNoFuture); + rules.add(dropForever); + + DruidExceptionMatcher.invalidInput().expectMessageContains( + StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", dropByP1M, dropByP1MNoFuture) + ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + + rules.clear(); rules.add(dropForever); rules.add(dropByP1M); @@ -316,6 +337,7 @@ public void testDatasourceRulesWithInvalidBroadcastRules() final ForeverBroadcastDistributionRule broadcastForever = new ForeverBroadcastDistributionRule(); final PeriodBroadcastDistributionRule broadcastPeriodPT1H = new PeriodBroadcastDistributionRule(new Period("PT1H"), true); + final PeriodBroadcastDistributionRule broadcastPeriodPT1HNoFuture = new PeriodBroadcastDistributionRule(new Period("PT1H"), false); final PeriodBroadcastDistributionRule broadcastPeriodPT2H = new PeriodBroadcastDistributionRule(new Period("PT2H"), true); final IntervalBroadcastDistributionRule broadcastInterval1 = new IntervalBroadcastDistributionRule( Intervals.of("2000-09-29T01:00:00Z/2050-10-30T01:00:00Z") @@ -342,6 +364,25 @@ public void testDatasourceRulesWithInvalidBroadcastRules() StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", broadcastPeriodPT2H, broadcastPeriodPT1H) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + rules.clear(); + rules.add(broadcastPeriodPT1H); + rules.add(broadcastPeriodPT1H); + rules.add(broadcastForever); + + DruidExceptionMatcher.invalidInput().expectMessageContains( + StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", broadcastPeriodPT1H, broadcastPeriodPT1H) + ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + + // Same interval with and without future. + rules.clear(); + rules.add(broadcastPeriodPT1H); + rules.add(broadcastPeriodPT1HNoFuture); + rules.add(broadcastForever); + + DruidExceptionMatcher.invalidInput().expectMessageContains( + StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", broadcastPeriodPT1H, broadcastPeriodPT1HNoFuture) + ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + rules.clear(); rules.add(broadcastPeriodPT1H); rules.add(broadcastPeriodPT2H); @@ -412,18 +453,25 @@ public void testSetDatasourceRulesWithValidRules() null, null ); - final PeriodLoadRule periodPT1H = new PeriodLoadRule(new Period("PT1H"), null, null, null); - final PeriodLoadRule periodP3M = new PeriodLoadRule(new Period("P3M"), null, null, null); - final PeriodLoadRule periodP6M = new PeriodLoadRule(new Period("P6M"), null, null, null); + final PeriodLoadRule periodPT1H = new PeriodLoadRule(new Period("PT1H"), true, null, null); + final PeriodLoadRule periodPT1HNoFuture = new PeriodLoadRule(new Period("PT1H"), false, null, null); + final PeriodLoadRule periodP3M = new PeriodLoadRule(new Period("P3M"), true, null, null); + final PeriodLoadRule periodP6M = new PeriodLoadRule(new Period("P6M"), true, null, null); + final PeriodDropRule dropByP1M = new PeriodDropRule(new Period("P1M"), true); + final PeriodDropRule dropByP1MNoFuture = new PeriodDropRule(new Period("P1M"), false); final ForeverLoadRule foreverLoadRule = new ForeverLoadRule(null, null); final ForeverDropRule foreverDropRule = new ForeverDropRule(); final PeriodBroadcastDistributionRule broadcastPeriodPT15m = new PeriodBroadcastDistributionRule(new Period("PT15m"), true); + final PeriodBroadcastDistributionRule broadcastPeriodPT15mNoFuture = new PeriodBroadcastDistributionRule(new Period("PT15m"), false); final List rules = new ArrayList<>(); rules.add(loadSmallInterval); rules.add(loadLargeInteval); rules.add(broadcastPeriodPT15m); + rules.add(periodPT1HNoFuture); rules.add(periodPT1H); + rules.add(dropByP1MNoFuture); + rules.add(dropByP1M); rules.add(periodP3M); rules.add(periodP6M); rules.add(foreverLoadRule); @@ -433,8 +481,12 @@ public void testSetDatasourceRulesWithValidRules() EasyMock.verify(databaseRuleManager); rules.clear(); + rules.add(broadcastPeriodPT15mNoFuture); rules.add(broadcastPeriodPT15m); + rules.add(periodPT1HNoFuture); rules.add(periodPT1H); + rules.add(dropByP1MNoFuture); + rules.add(dropByP1M); rules.add(periodP3M); rules.add(periodP6M); rules.add(loadSmallInterval); From 20133fceb1201f02a93508a6c4aa08ba01e56c7f Mon Sep 17 00:00:00 2001 From: Abhishek Balaji Radhakrishnan Date: Wed, 20 Sep 2023 14:23:21 -0700 Subject: [PATCH 5/9] Some test cleanup. --- .../druid/server/coordinator/rules/Rules.java | 5 + .../druid/server/http/RulesResourceTest.java | 107 +++++++++--------- 2 files changed, 59 insertions(+), 53 deletions(-) diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java index a25a1b363f4a..c6ea531f6a66 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java @@ -25,7 +25,11 @@ import org.joda.time.DateTime; import org.joda.time.Interval; import org.joda.time.Period; +import org.joda.time.base.BaseInterval; +import java.util.ArrayList; +import java.util.Comparator; +import java.util.LinkedList; import java.util.List; public class Rules @@ -60,6 +64,7 @@ public static void validateRules(final List 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); diff --git a/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java b/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java index 909557c73f9a..fd2234e8b34b 100644 --- a/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java +++ b/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java @@ -38,6 +38,7 @@ import org.apache.druid.server.coordinator.rules.PeriodDropRule; import org.apache.druid.server.coordinator.rules.PeriodLoadRule; import org.apache.druid.server.coordinator.rules.Rule; +import org.apache.druid.server.coordinator.rules.Rules; import org.easymock.EasyMock; import org.joda.time.Interval; import org.joda.time.Period; @@ -281,21 +282,21 @@ public void testDatasourceRulesWithInvalidDropRules() final RulesResource rulesResource = new RulesResource(databaseRuleManager, auditManager); final HttpServletRequest req = EasyMock.createMock(HttpServletRequest.class); - final PeriodDropBeforeRule dropBeforeP3M = new PeriodDropBeforeRule(new Period("P3M")); - final PeriodDropBeforeRule dropBeforeP6M = new PeriodDropBeforeRule(new Period("P6M")); + final PeriodDropBeforeRule dropBeforeByP3M = new PeriodDropBeforeRule(new Period("P3M")); + final PeriodDropBeforeRule dropBeforeByP6M = new PeriodDropBeforeRule(new Period("P6M")); final PeriodDropRule dropByP1M = new PeriodDropRule(new Period("P1M"), true); final PeriodDropRule dropByP1MNoFuture = new PeriodDropRule(new Period("P1M"), false); final PeriodDropRule dropByP2M = new PeriodDropRule(new Period("P2M"), true); final ForeverDropRule dropForever = new ForeverDropRule(); final List rules = new ArrayList<>(); - rules.add(dropBeforeP3M); - rules.add(dropBeforeP6M); + rules.add(dropBeforeByP3M); + rules.add(dropBeforeByP6M); rules.add(dropByP1M); rules.add(dropForever); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", dropBeforeP3M, dropBeforeP6M) + StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", dropBeforeByP3M, dropBeforeByP6M) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); rules.clear(); @@ -336,9 +337,9 @@ public void testDatasourceRulesWithInvalidBroadcastRules() final HttpServletRequest req = EasyMock.createMock(HttpServletRequest.class); final ForeverBroadcastDistributionRule broadcastForever = new ForeverBroadcastDistributionRule(); - final PeriodBroadcastDistributionRule broadcastPeriodPT1H = new PeriodBroadcastDistributionRule(new Period("PT1H"), true); - final PeriodBroadcastDistributionRule broadcastPeriodPT1HNoFuture = new PeriodBroadcastDistributionRule(new Period("PT1H"), false); - final PeriodBroadcastDistributionRule broadcastPeriodPT2H = new PeriodBroadcastDistributionRule(new Period("PT2H"), true); + final PeriodBroadcastDistributionRule broadcastPT1H = new PeriodBroadcastDistributionRule(new Period("PT1H"), true); + final PeriodBroadcastDistributionRule broadcastPT1HNoFuture = new PeriodBroadcastDistributionRule(new Period("PT1H"), false); + final PeriodBroadcastDistributionRule broadcastPT2H = new PeriodBroadcastDistributionRule(new Period("PT2H"), true); final IntervalBroadcastDistributionRule broadcastInterval1 = new IntervalBroadcastDistributionRule( Intervals.of("2000-09-29T01:00:00Z/2050-10-30T01:00:00Z") ); @@ -356,36 +357,36 @@ public void testDatasourceRulesWithInvalidBroadcastRules() ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); rules.clear(); - rules.add(broadcastPeriodPT2H); - rules.add(broadcastPeriodPT1H); + rules.add(broadcastPT2H); + rules.add(broadcastPT1H); rules.add(broadcastForever); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", broadcastPeriodPT2H, broadcastPeriodPT1H) + StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", broadcastPT2H, broadcastPT1H) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); rules.clear(); - rules.add(broadcastPeriodPT1H); - rules.add(broadcastPeriodPT1H); + rules.add(broadcastPT1H); + rules.add(broadcastPT1H); rules.add(broadcastForever); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", broadcastPeriodPT1H, broadcastPeriodPT1H) + StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", broadcastPT1H, broadcastPT1H) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); // Same interval with and without future. rules.clear(); - rules.add(broadcastPeriodPT1H); - rules.add(broadcastPeriodPT1HNoFuture); + rules.add(broadcastPT1H); + rules.add(broadcastPT1HNoFuture); rules.add(broadcastForever); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", broadcastPeriodPT1H, broadcastPeriodPT1HNoFuture) + StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", broadcastPT1H, broadcastPT1HNoFuture) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); rules.clear(); - rules.add(broadcastPeriodPT1H); - rules.add(broadcastPeriodPT2H); + rules.add(broadcastPT1H); + rules.add(broadcastPT2H); rules.add(broadcastInterval2); rules.add(broadcastForever); rules.add(broadcastInterval1); @@ -413,25 +414,25 @@ public void testSetDatasourceRulesWithDifferentInvalidRules() null, null ); - final PeriodLoadRule periodPT1H = new PeriodLoadRule(new Period("PT1H"), null, null, null); - final PeriodLoadRule periodP3M = new PeriodLoadRule(new Period("P3M"), null, null, null); - final PeriodLoadRule periodP6M = new PeriodLoadRule(new Period("P6M"), null, null, null); - final ForeverLoadRule foreverLoadRule = new ForeverLoadRule(null, null); - final ForeverDropRule foreverDropRule = new ForeverDropRule(); - final PeriodBroadcastDistributionRule broadcastPeriodPT15m = new PeriodBroadcastDistributionRule(new Period("PT15m"), true); + final PeriodLoadRule loadPT1H = new PeriodLoadRule(new Period("PT1H"), null, null, null); + final PeriodLoadRule loadP3M = new PeriodLoadRule(new Period("P3M"), null, null, null); + final PeriodLoadRule loadP6M = new PeriodLoadRule(new Period("P6M"), null, null, null); + final ForeverLoadRule loadForever = new ForeverLoadRule(null, null); + final ForeverDropRule dropForever = new ForeverDropRule(); + final PeriodBroadcastDistributionRule broadcastPT15m = new PeriodBroadcastDistributionRule(new Period("PT15m"), true); final List rules = new ArrayList<>(); rules.add(loadSmallInterval); rules.add(loadLargeInteval); - rules.add(broadcastPeriodPT15m); - rules.add(periodPT1H); - rules.add(periodP3M); - rules.add(periodP6M); - rules.add(foreverLoadRule); - rules.add(foreverDropRule); + rules.add(broadcastPT15m); + rules.add(loadPT1H); + rules.add(loadP3M); + rules.add(loadP6M); + rules.add(loadForever); + rules.add(dropForever); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", foreverLoadRule, foreverDropRule) + StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", loadForever, dropForever) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); } @@ -453,45 +454,45 @@ public void testSetDatasourceRulesWithValidRules() null, null ); - final PeriodLoadRule periodPT1H = new PeriodLoadRule(new Period("PT1H"), true, null, null); - final PeriodLoadRule periodPT1HNoFuture = new PeriodLoadRule(new Period("PT1H"), false, null, null); - final PeriodLoadRule periodP3M = new PeriodLoadRule(new Period("P3M"), true, null, null); - final PeriodLoadRule periodP6M = new PeriodLoadRule(new Period("P6M"), true, null, null); + final PeriodLoadRule loadPT1H = new PeriodLoadRule(new Period("PT1H"), true, null, null); + final PeriodLoadRule loadPT1HNoFuture = new PeriodLoadRule(new Period("PT1H"), false, null, null); + final PeriodLoadRule loadP3M = new PeriodLoadRule(new Period("P3M"), true, null, null); + final PeriodLoadRule loadP6M = new PeriodLoadRule(new Period("P6M"), true, null, null); final PeriodDropRule dropByP1M = new PeriodDropRule(new Period("P1M"), true); final PeriodDropRule dropByP1MNoFuture = new PeriodDropRule(new Period("P1M"), false); - final ForeverLoadRule foreverLoadRule = new ForeverLoadRule(null, null); - final ForeverDropRule foreverDropRule = new ForeverDropRule(); - final PeriodBroadcastDistributionRule broadcastPeriodPT15m = new PeriodBroadcastDistributionRule(new Period("PT15m"), true); - final PeriodBroadcastDistributionRule broadcastPeriodPT15mNoFuture = new PeriodBroadcastDistributionRule(new Period("PT15m"), false); + final ForeverLoadRule loadForever = new ForeverLoadRule(null, null); + final ForeverDropRule dropForever = new ForeverDropRule(); + final PeriodBroadcastDistributionRule broadcastPT15m = new PeriodBroadcastDistributionRule(new Period("PT15m"), true); + final PeriodBroadcastDistributionRule broadcastPT15mNoFuture = new PeriodBroadcastDistributionRule(new Period("PT15m"), false); final List rules = new ArrayList<>(); rules.add(loadSmallInterval); rules.add(loadLargeInteval); - rules.add(broadcastPeriodPT15m); - rules.add(periodPT1HNoFuture); - rules.add(periodPT1H); + rules.add(broadcastPT15m); + rules.add(loadPT1HNoFuture); + rules.add(loadPT1H); rules.add(dropByP1MNoFuture); rules.add(dropByP1M); - rules.add(periodP3M); - rules.add(periodP6M); - rules.add(foreverLoadRule); + rules.add(loadP3M); + rules.add(loadP6M); + rules.add(loadForever); final Response resp = rulesResource.setDatasourceRules("dataSource1", rules, null, null, EasyMock.createMock(HttpServletRequest.class)); Assert.assertEquals(200, resp.getStatus()); EasyMock.verify(databaseRuleManager); rules.clear(); - rules.add(broadcastPeriodPT15mNoFuture); - rules.add(broadcastPeriodPT15m); - rules.add(periodPT1HNoFuture); - rules.add(periodPT1H); + rules.add(broadcastPT15mNoFuture); + rules.add(broadcastPT15m); + rules.add(loadPT1HNoFuture); + rules.add(loadPT1H); rules.add(dropByP1MNoFuture); rules.add(dropByP1M); - rules.add(periodP3M); - rules.add(periodP6M); + rules.add(loadP3M); + rules.add(loadP6M); rules.add(loadSmallInterval); rules.add(loadLargeInteval); - rules.add(foreverDropRule); + rules.add(dropForever); final Response resp2 = rulesResource.setDatasourceRules("dataSource1", rules, null, null, EasyMock.createMock(HttpServletRequest.class)); Assert.assertEquals(200, resp2.getStatus()); From f8975e26fcbcf993f6cc17af8f0e3f8c7347cbad Mon Sep 17 00:00:00 2001 From: Abhishek Balaji Radhakrishnan Date: Wed, 20 Sep 2023 18:21:42 -0700 Subject: [PATCH 6/9] fix and add more tests. --- .../druid/server/coordinator/rules/Rules.java | 46 +++---- .../druid/server/http/RulesResourceTest.java | 113 +++++++++++------- 2 files changed, 92 insertions(+), 67 deletions(-) diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java index c6ea531f6a66..540bd1842ff7 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java @@ -25,11 +25,7 @@ import org.joda.time.DateTime; import org.joda.time.Interval; import org.joda.time.Period; -import org.joda.time.base.BaseInterval; -import java.util.ArrayList; -import java.util.Comparator; -import java.util.LinkedList; import java.util.List; public class Rules @@ -55,7 +51,7 @@ 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 + * fully covers another subsequent rules' interval in the list. Rules that will be evaluated at some point * are considered to be legitimate. * @param rules Datasource rules. */ @@ -66,25 +62,31 @@ public static void validateRules(final List rules) } final DateTime now = DateTimes.nowUtc(); - for (int i = 0; i < rules.size() - 1; i++) { + for (int i = 0; i < rules.size(); 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 - ); + + for (int j = i + 1; j < rules.size(); j++) { + final Rule nextRule = rules.get(j); + final Interval nextInterval = nextRule.getEligibleInterval(now); + if (currInterval.contains(nextInterval)) { + // If the current rule has eternity, 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 fully contains the interval for rule[%s]." + + " i.e., interval[%s] hides interval[%s]. Please fix the rules and retry.", + currRule, + nextRule, + currInterval, + nextInterval + ); + } } } } diff --git a/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java b/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java index fd2234e8b34b..7ec3b5acc351 100644 --- a/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java +++ b/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java @@ -32,13 +32,13 @@ import org.apache.druid.server.coordinator.rules.ForeverDropRule; import org.apache.druid.server.coordinator.rules.ForeverLoadRule; import org.apache.druid.server.coordinator.rules.IntervalBroadcastDistributionRule; +import org.apache.druid.server.coordinator.rules.IntervalDropRule; import org.apache.druid.server.coordinator.rules.IntervalLoadRule; import org.apache.druid.server.coordinator.rules.PeriodBroadcastDistributionRule; import org.apache.druid.server.coordinator.rules.PeriodDropBeforeRule; import org.apache.druid.server.coordinator.rules.PeriodDropRule; import org.apache.druid.server.coordinator.rules.PeriodLoadRule; import org.apache.druid.server.coordinator.rules.Rule; -import org.apache.druid.server.coordinator.rules.Rules; import org.easymock.EasyMock; import org.joda.time.Interval; import org.joda.time.Period; @@ -191,20 +191,20 @@ public void testSetDatasourceRulesWithInvalidLoadRules() final RulesResource rulesResource = new RulesResource(databaseRuleManager, auditManager); final HttpServletRequest req = EasyMock.createMock(HttpServletRequest.class); - final IntervalLoadRule loadInterval1 = new IntervalLoadRule( - Intervals.of("2023-07-29T01:00:00Z/2023-12-30T01:00:00Z"), + final PeriodLoadRule loadPT1H = new PeriodLoadRule(new Period("PT1H"), true, null, null); + final PeriodLoadRule loadPT1HNoFuture = new PeriodLoadRule(new Period("PT1H"), false, null, null); + final PeriodLoadRule loadP3M = new PeriodLoadRule(new Period("P3M"), true, null, null); + final PeriodLoadRule loadP6M = new PeriodLoadRule(new Period("P6M"), true, null, null); + final IntervalLoadRule loadInterval2020To2023 = new IntervalLoadRule( + Intervals.of("2020/2023"), null, null ); - final IntervalLoadRule loadInterval2 = new IntervalLoadRule( - Intervals.of("2023-09-29T01:00:00Z/2023-10-30T01:00:00Z"), + final IntervalLoadRule loadInterval2021To2022 = new IntervalLoadRule( + Intervals.of("2021/2022"), null, null ); - final PeriodLoadRule loadPT1H = new PeriodLoadRule(new Period("PT1H"), true, null, null); - final PeriodLoadRule loadPT1HNoFuture = new PeriodLoadRule(new Period("PT1H"), false, null, null); - final PeriodLoadRule loadP3M = new PeriodLoadRule(new Period("P3M"), true, null, null); - final PeriodLoadRule loadP6M = new PeriodLoadRule(new Period("P6M"), true, null, null); final ForeverLoadRule loadForever = new ForeverLoadRule(null, null); final List rules = new ArrayList<>(); @@ -213,7 +213,7 @@ public void testSetDatasourceRulesWithInvalidLoadRules() rules.add(loadForever); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", loadP6M, loadP3M) + StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", loadP6M, loadP3M) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); rules.clear(); @@ -222,17 +222,17 @@ public void testSetDatasourceRulesWithInvalidLoadRules() rules.add(loadP6M); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", loadForever, loadP6M) + StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", loadForever, loadP6M) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); rules.clear(); rules.add(loadForever); rules.add(loadPT1H); - rules.add(loadInterval2); + rules.add(loadInterval2021To2022); rules.add(loadP6M); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", loadForever, loadPT1H) + StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", loadForever, loadPT1H) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); rules.clear(); @@ -241,7 +241,7 @@ public void testSetDatasourceRulesWithInvalidLoadRules() rules.add(loadP6M); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", loadPT1H, loadPT1HNoFuture) + StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", loadPT1H, loadPT1HNoFuture) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); rules.clear(); @@ -250,27 +250,38 @@ public void testSetDatasourceRulesWithInvalidLoadRules() rules.add(loadP6M); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", loadPT1H, loadPT1H) + StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", loadPT1H, loadPT1H) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); rules.clear(); - rules.add(loadInterval1); - rules.add(loadInterval2); + rules.add(loadInterval2020To2023); + rules.add(loadInterval2021To2022); rules.add(loadP6M); rules.add(loadForever); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", loadInterval1, loadInterval2) + StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", loadInterval2020To2023, loadInterval2021To2022) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); rules.clear(); rules.add(loadP6M); - rules.add(loadInterval1); + rules.add(loadInterval2021To2022); + rules.add(loadP3M); + rules.add(loadInterval2020To2023); + + DruidExceptionMatcher.invalidInput().expectMessageContains( + StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", loadP6M, loadP3M) + ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + + + rules.clear(); + rules.add(loadP6M); + rules.add(loadInterval2020To2023); rules.add(loadForever); - rules.add(loadInterval2); + rules.add(loadInterval2021To2022); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", loadForever, loadInterval2) + StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", loadInterval2020To2023, loadInterval2021To2022) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); } @@ -285,6 +296,8 @@ public void testDatasourceRulesWithInvalidDropRules() final PeriodDropBeforeRule dropBeforeByP3M = new PeriodDropBeforeRule(new Period("P3M")); final PeriodDropBeforeRule dropBeforeByP6M = new PeriodDropBeforeRule(new Period("P6M")); final PeriodDropRule dropByP1M = new PeriodDropRule(new Period("P1M"), true); + final IntervalDropRule dropInterval2000To2020 = new IntervalDropRule(Intervals.of("2000/2020")); + final IntervalDropRule dropInterval2010To2020 = new IntervalDropRule(Intervals.of("2010/2020")); final PeriodDropRule dropByP1MNoFuture = new PeriodDropRule(new Period("P1M"), false); final PeriodDropRule dropByP2M = new PeriodDropRule(new Period("P2M"), true); final ForeverDropRule dropForever = new ForeverDropRule(); @@ -296,7 +309,7 @@ public void testDatasourceRulesWithInvalidDropRules() rules.add(dropForever); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", dropBeforeByP3M, dropBeforeByP6M) + StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", dropBeforeByP3M, dropBeforeByP6M) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); rules.clear(); @@ -305,7 +318,17 @@ public void testDatasourceRulesWithInvalidDropRules() rules.add(dropForever); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", dropByP2M, dropByP1M) + StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", dropByP2M, dropByP1M) + ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + + rules.clear(); + rules.add(dropInterval2000To2020); + rules.add(dropByP1M); + rules.add(dropByP2M); + rules.add(dropInterval2010To2020); + + DruidExceptionMatcher.invalidInput().expectMessageContains( + StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", dropInterval2000To2020, dropInterval2010To2020) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); rules.clear(); @@ -314,7 +337,7 @@ public void testDatasourceRulesWithInvalidDropRules() rules.add(dropForever); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", dropByP1M, dropByP1MNoFuture) + StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", dropByP1M, dropByP1MNoFuture) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); @@ -324,7 +347,7 @@ public void testDatasourceRulesWithInvalidDropRules() rules.add(dropByP2M); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", dropForever, dropByP1M) + StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", dropForever, dropByP1M) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); } @@ -340,20 +363,20 @@ public void testDatasourceRulesWithInvalidBroadcastRules() final PeriodBroadcastDistributionRule broadcastPT1H = new PeriodBroadcastDistributionRule(new Period("PT1H"), true); final PeriodBroadcastDistributionRule broadcastPT1HNoFuture = new PeriodBroadcastDistributionRule(new Period("PT1H"), false); final PeriodBroadcastDistributionRule broadcastPT2H = new PeriodBroadcastDistributionRule(new Period("PT2H"), true); - final IntervalBroadcastDistributionRule broadcastInterval1 = new IntervalBroadcastDistributionRule( - Intervals.of("2000-09-29T01:00:00Z/2050-10-30T01:00:00Z") + final IntervalBroadcastDistributionRule broadcastInterval2000To2050 = new IntervalBroadcastDistributionRule( + Intervals.of("2000/2050") ); - final IntervalBroadcastDistributionRule broadcastInterval2 = new IntervalBroadcastDistributionRule( - Intervals.of("2010-09-29T01:00:00Z/2020-10-30T01:00:00Z") + final IntervalBroadcastDistributionRule broadcastInterval2010To2020 = new IntervalBroadcastDistributionRule( + Intervals.of("2010/2020") ); final List rules = new ArrayList<>(); - rules.add(broadcastInterval1); - rules.add(broadcastInterval2); + rules.add(broadcastInterval2000To2050); + rules.add(broadcastInterval2010To2020); rules.add(broadcastForever); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", broadcastInterval1, broadcastInterval2) + StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", broadcastInterval2000To2050, broadcastInterval2010To2020) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); rules.clear(); @@ -362,7 +385,7 @@ public void testDatasourceRulesWithInvalidBroadcastRules() rules.add(broadcastForever); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", broadcastPT2H, broadcastPT1H) + StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", broadcastPT2H, broadcastPT1H) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); rules.clear(); @@ -371,7 +394,7 @@ public void testDatasourceRulesWithInvalidBroadcastRules() rules.add(broadcastForever); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", broadcastPT1H, broadcastPT1H) + StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", broadcastPT1H, broadcastPT1H) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); // Same interval with and without future. @@ -381,18 +404,18 @@ public void testDatasourceRulesWithInvalidBroadcastRules() rules.add(broadcastForever); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", broadcastPT1H, broadcastPT1HNoFuture) + StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", broadcastPT1H, broadcastPT1HNoFuture) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); rules.clear(); rules.add(broadcastPT1H); rules.add(broadcastPT2H); - rules.add(broadcastInterval2); + rules.add(broadcastInterval2010To2020); rules.add(broadcastForever); - rules.add(broadcastInterval1); + rules.add(broadcastInterval2000To2050); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", broadcastForever, broadcastInterval1) + StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", broadcastForever, broadcastInterval2000To2050) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); } @@ -404,13 +427,13 @@ public void testSetDatasourceRulesWithDifferentInvalidRules() final RulesResource rulesResource = new RulesResource(databaseRuleManager, auditManager); final HttpServletRequest req = EasyMock.createMock(HttpServletRequest.class); - final IntervalLoadRule loadLargeInteval = new IntervalLoadRule( - Intervals.of("1980-07-29T01:00:00Z/2050-12-30T01:00:00Z"), + final IntervalLoadRule loadInterval1980To2050 = new IntervalLoadRule( + Intervals.of("1980/2050"), null, null ); - final IntervalLoadRule loadSmallInterval = new IntervalLoadRule( - Intervals.of("2020-09-29T01:00:00Z/2025-10-30T01:00:00Z"), + final IntervalLoadRule loadInterval2020To2025 = new IntervalLoadRule( + Intervals.of("2020/2025"), null, null ); @@ -422,9 +445,9 @@ public void testSetDatasourceRulesWithDifferentInvalidRules() final PeriodBroadcastDistributionRule broadcastPT15m = new PeriodBroadcastDistributionRule(new Period("PT15m"), true); final List rules = new ArrayList<>(); - rules.add(loadSmallInterval); - rules.add(loadLargeInteval); + rules.add(loadInterval2020To2025); rules.add(broadcastPT15m); + rules.add(loadInterval1980To2050); rules.add(loadPT1H); rules.add(loadP3M); rules.add(loadP6M); @@ -432,7 +455,7 @@ public void testSetDatasourceRulesWithDifferentInvalidRules() rules.add(dropForever); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that contains interval for rule[%s].", loadForever, dropForever) + StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", loadForever, dropForever) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); } From 320cd90ccf4156fe9e53c9b784be1a9fb36fcc35 Mon Sep 17 00:00:00 2001 From: Abhishek Balaji Radhakrishnan Date: Tue, 26 Sep 2023 22:06:29 -0700 Subject: [PATCH 7/9] Minor adjustments for consistency --- .../druid/server/coordinator/rules/PeriodDropBeforeRule.java | 2 ++ .../apache/druid/server/coordinator/rules/PeriodDropRule.java | 4 +++- .../apache/druid/server/coordinator/rules/PeriodLoadRule.java | 4 +++- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropBeforeRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropBeforeRule.java index 07e220a1dfdc..9b9b2c0a72aa 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropBeforeRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropBeforeRule.java @@ -22,6 +22,7 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; import org.apache.druid.java.util.common.DateTimes; +import org.apache.druid.java.util.common.JodaUtils; import org.apache.druid.timeline.DataSegment; import org.joda.time.DateTime; import org.joda.time.Interval; @@ -68,6 +69,7 @@ public boolean appliesTo(Interval theInterval, DateTime referenceTimestamp) @Override public Interval getEligibleInterval(DateTime referenceTimestamp) { +// return new Interval(DateTimes.utc(JodaUtils.MIN_INSTANT), referenceTimestamp.minus(period)); return new Interval(DateTimes.utc(Long.MIN_VALUE), referenceTimestamp.minus(period)); } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropRule.java index c999bb01ad96..47744dffb8ba 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropRule.java @@ -21,6 +21,8 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.druid.java.util.common.DateTimes; +import org.apache.druid.java.util.common.JodaUtils; import org.apache.druid.timeline.DataSegment; import org.joda.time.DateTime; import org.joda.time.Interval; @@ -85,7 +87,7 @@ public boolean appliesTo(Interval theInterval, DateTime referenceTimestamp) @Override public Interval getEligibleInterval(DateTime referenceTimestamp) { - return includeFuture ? new Interval(referenceTimestamp.minus(period), referenceTimestamp.plus(period)) + return includeFuture ? new Interval(referenceTimestamp.minus(period), DateTimes.utc(JodaUtils.MAX_INSTANT)) : new Interval(referenceTimestamp.minus(period), referenceTimestamp); } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java index 0481195ec0a7..0e6f32f1cadb 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java @@ -21,6 +21,8 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.druid.java.util.common.DateTimes; +import org.apache.druid.java.util.common.JodaUtils; import org.apache.druid.timeline.DataSegment; import org.joda.time.DateTime; import org.joda.time.Interval; @@ -86,7 +88,7 @@ public boolean appliesTo(Interval interval, DateTime referenceTimestamp) @Override public Interval getEligibleInterval(DateTime referenceTimestamp) { - return includeFuture ? new Interval(referenceTimestamp.minus(period), referenceTimestamp.plus(period)) + return includeFuture ? new Interval(referenceTimestamp.minus(period), DateTimes.utc(JodaUtils.MAX_INSTANT)) : new Interval(referenceTimestamp.minus(period), referenceTimestamp); } From d729fdff375348ac617744e57bbd2d2cb9504bae Mon Sep 17 00:00:00 2001 From: Abhishek Balaji Radhakrishnan Date: Tue, 26 Sep 2023 22:07:09 -0700 Subject: [PATCH 8/9] Add separate test classes. Do some code cleanup. --- .../PeriodBroadcastDistributionRule.java | 4 +- .../rules/PeriodDropBeforeRule.java | 11 +- .../coordinator/rules/PeriodDropRule.java | 3 +- .../coordinator/rules/PeriodLoadRule.java | 3 +- .../druid/server/coordinator/rules/Rule.java | 6 +- .../druid/server/coordinator/rules/Rules.java | 23 +- .../rules/RulesEligibleIntervalTest.java | 195 ++++++++++ .../server/coordinator/rules/RulesTest.java | 149 ++++++++ .../druid/server/http/RulesResourceTest.java | 354 ++---------------- 9 files changed, 408 insertions(+), 340 deletions(-) create mode 100644 server/src/test/java/org/apache/druid/server/coordinator/rules/RulesEligibleIntervalTest.java create mode 100644 server/src/test/java/org/apache/druid/server/coordinator/rules/RulesTest.java diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodBroadcastDistributionRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodBroadcastDistributionRule.java index 5de02add3316..17bbf7be789b 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodBroadcastDistributionRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodBroadcastDistributionRule.java @@ -21,6 +21,8 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.druid.java.util.common.DateTimes; +import org.apache.druid.java.util.common.JodaUtils; import org.apache.druid.timeline.DataSegment; import org.joda.time.DateTime; import org.joda.time.Interval; @@ -68,7 +70,7 @@ public boolean appliesTo(Interval interval, DateTime referenceTimestamp) @Override public Interval getEligibleInterval(DateTime referenceTimestamp) { - return includeFuture ? new Interval(referenceTimestamp.minus(period), referenceTimestamp.plus(period)) + return includeFuture ? new Interval(referenceTimestamp.minus(period), DateTimes.utc(JodaUtils.MAX_INSTANT)) : new Interval(referenceTimestamp.minus(period), referenceTimestamp); } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropBeforeRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropBeforeRule.java index 9b9b2c0a72aa..7198e5af7006 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropBeforeRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropBeforeRule.java @@ -22,7 +22,6 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; import org.apache.druid.java.util.common.DateTimes; -import org.apache.druid.java.util.common.JodaUtils; import org.apache.druid.timeline.DataSegment; import org.joda.time.DateTime; import org.joda.time.Interval; @@ -69,8 +68,14 @@ public boolean appliesTo(Interval theInterval, DateTime referenceTimestamp) @Override public Interval getEligibleInterval(DateTime referenceTimestamp) { -// return new Interval(DateTimes.utc(JodaUtils.MIN_INSTANT), referenceTimestamp.minus(period)); - return new Interval(DateTimes.utc(Long.MIN_VALUE), referenceTimestamp.minus(period)); + final DateTime end = referenceTimestamp.minus(period); + if (end.isBefore(DateTimes.MIN)) { + // We use Long.MIN_VALUE as the start here (instead of DateTimes.MIN) when end is < DateTimes.MIN because the + // resulting interval will be invalid where start > end. This is true for referenceTimestamp = DateTimes.MIN. + return new Interval(DateTimes.utc(Long.MIN_VALUE), end); + } else { + return new Interval(DateTimes.MIN, end); + } } @Override diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropRule.java index 47744dffb8ba..e92e55a3c9a8 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodDropRule.java @@ -22,7 +22,6 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; import org.apache.druid.java.util.common.DateTimes; -import org.apache.druid.java.util.common.JodaUtils; import org.apache.druid.timeline.DataSegment; import org.joda.time.DateTime; import org.joda.time.Interval; @@ -87,7 +86,7 @@ public boolean appliesTo(Interval theInterval, DateTime referenceTimestamp) @Override public Interval getEligibleInterval(DateTime referenceTimestamp) { - return includeFuture ? new Interval(referenceTimestamp.minus(period), DateTimes.utc(JodaUtils.MAX_INSTANT)) + return includeFuture ? new Interval(referenceTimestamp.minus(period), DateTimes.MAX) : new Interval(referenceTimestamp.minus(period), referenceTimestamp); } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java index 0e6f32f1cadb..b799a664eb45 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java @@ -22,7 +22,6 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; import org.apache.druid.java.util.common.DateTimes; -import org.apache.druid.java.util.common.JodaUtils; import org.apache.druid.timeline.DataSegment; import org.joda.time.DateTime; import org.joda.time.Interval; @@ -88,7 +87,7 @@ public boolean appliesTo(Interval interval, DateTime referenceTimestamp) @Override public Interval getEligibleInterval(DateTime referenceTimestamp) { - return includeFuture ? new Interval(referenceTimestamp.minus(period), DateTimes.utc(JodaUtils.MAX_INSTANT)) + return includeFuture ? new Interval(referenceTimestamp.minus(period), DateTimes.MAX) : new Interval(referenceTimestamp.minus(period), referenceTimestamp); } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/Rule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/Rule.java index c34b086484c8..72c02ceb5416 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/Rule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/Rule.java @@ -52,9 +52,9 @@ public interface Rule void run(DataSegment segment, SegmentActionHandler segmentHandler); /** - * Return an eligible interval from the reference timestamp. Implementations - * must return a valid interval based on the rule type. - * @param referenceTimestamp base timestamp + * Returns the interval eligible for this rule. The interval must be computed based on the rule type + * optionally using {@code referenceTimestamp}. {@code referenceTimestamp} must be a timestamp + * between [{@link org.apache.druid.java.util.common.DateTimes.MIN}, {@link org.apache.druid.java.util.common.DateTimes.MAX}). */ Interval getEligibleInterval(DateTime referenceTimestamp); } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java index 540bd1842ff7..bf068de088c2 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java @@ -50,10 +50,22 @@ private Rules() } /** - * Validate rules. This method throws an exception if a rule contain an interval that - * fully covers another subsequent rules' interval in the list. Rules that will be evaluated at some point - * are considered to be legitimate. - * @param rules Datasource rules. + * Validates the given list of retention rules for a datasource (or cluster default). + * A rule is considered valid only if it will be evaluated at some point, i.e. its eligible interval + * is not fully covered by the eligible interval of any preceding rule in the list. + *

+ * Consider two rules r1 and r2. Assume r1 and r2's eligible intervals at the time when the rules are evaluated are + * i1 and i2 respectively. r1 and r2 are invalid if: + *

    + *
  • i1 is eternity. i.e., eternity fully covers i2 and any other interval that follows it. Or
    • + *
  • i1 fully contains i2 and
    • + *
  • r1's eligible interval at i1's start and end fully contain r2's eligible interval at i1's start and end + * respectively. This boundary check is used to identify rules that will fire at some point. i.e., period based rules + * will return distinct eligible intervals at the boundaries, whereas broadcast and interval based rules will return + * fixed intervals regardless of the boundary.
    • + *
+ *

+ * @throws org.apache.druid.error.DruidException with error code "invalidInput" if any of the given rules is not valid. */ public static void validateRules(final List rules) { @@ -70,9 +82,6 @@ public static void validateRules(final List rules) final Rule nextRule = rules.get(j); final Interval nextInterval = nextRule.getEligibleInterval(now); if (currInterval.contains(nextInterval)) { - // If the current rule has eternity, 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())) diff --git a/server/src/test/java/org/apache/druid/server/coordinator/rules/RulesEligibleIntervalTest.java b/server/src/test/java/org/apache/druid/server/coordinator/rules/RulesEligibleIntervalTest.java new file mode 100644 index 000000000000..83fafbde0f34 --- /dev/null +++ b/server/src/test/java/org/apache/druid/server/coordinator/rules/RulesEligibleIntervalTest.java @@ -0,0 +1,195 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.server.coordinator.rules; + +import com.google.common.collect.ImmutableMap; +import org.apache.druid.java.util.common.DateTimes; +import org.apache.druid.java.util.common.Intervals; +import org.apache.druid.java.util.common.JodaUtils; +import org.joda.time.DateTime; +import org.joda.time.Interval; +import org.joda.time.Period; +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +@RunWith(Parameterized.class) +public class RulesEligibleIntervalTest +{ + private final DateTime referenceTime; + + @Parameterized.Parameters + public static Object[] getReferenceTimestamps() + { + final DateTime now = DateTimes.nowUtc(); + return new Object[]{ + now, + now.minusYears(5), + now.plusYears(10), + DateTimes.utc(0), + DateTimes.utc(JodaUtils.MIN_INSTANT + 1), + DateTimes.utc(JodaUtils.MIN_INSTANT), + DateTimes.utc(JodaUtils.MAX_INSTANT - 1), + DateTimes.utc(JodaUtils.MAX_INSTANT) + }; + } + + public RulesEligibleIntervalTest(final DateTime referenceTimestamp) + { + this.referenceTime = referenceTimestamp; + } + + @Test + public void testPeriodLoadEligibleInterval() + { + final Period period = new Period("P50Y"); + final PeriodLoadRule loadPT1H = new PeriodLoadRule( + period, + true, + null, + null + ); + Assert.assertEquals( + new Interval(this.referenceTime.minus(period), DateTimes.MAX), + loadPT1H.getEligibleInterval(this.referenceTime) + ); + } + + @Test + public void testPeriodLoadExcludingFutureEligibleInterval() + { + final Period period = new Period("PT1H"); + final PeriodLoadRule rule = new PeriodLoadRule( + period, + false, + null, + null + ); + Assert.assertEquals(new Interval(period, this.referenceTime), rule.getEligibleInterval(this.referenceTime)); + } + + @Test + public void testIntervalLoadEligibleInterval() + { + final Interval interval = Intervals.of("2000/3000"); + final IntervalLoadRule rule = new IntervalLoadRule( + interval, + null, + null + ); + Assert.assertEquals(interval, rule.getEligibleInterval(this.referenceTime)); + } + + @Test + public void testForeverLoadEligibleInterval() + { + final ForeverLoadRule rule = new ForeverLoadRule(ImmutableMap.of(), false); + Assert.assertEquals(Intervals.ETERNITY, rule.getEligibleInterval(this.referenceTime)); + } + + @Test + public void testPeriodDropEligibleInterval() + { + final Period period = new Period("P5000Y"); + final PeriodDropRule rule = new PeriodDropRule( + period, + true + ); + Assert.assertEquals( + new Interval(this.referenceTime.minus(period), DateTimes.utc(JodaUtils.MAX_INSTANT)), + rule.getEligibleInterval(this.referenceTime) + ); + } + + @Test + public void testPeriodDropExcludingFutureEligibleInterval() + { + final Period period = new Period("P50Y"); + final PeriodDropRule rule = new PeriodDropRule( + period, + false + ); + Assert.assertEquals( + new Interval(this.referenceTime.minus(period), this.referenceTime), + rule.getEligibleInterval(this.referenceTime) + ); + } + + @Test + public void testPeriodDropBeforeEligibleInterval() + { + final Period period = new Period("P50Y"); + final PeriodDropBeforeRule rule = new PeriodDropBeforeRule(period); + + if (this.referenceTime.minus(period).isBefore(DateTimes.MIN)) { + Assert.assertEquals( + new Interval(DateTimes.utc(Long.MIN_VALUE), this.referenceTime.minus(period)), + rule.getEligibleInterval(this.referenceTime) + ); + } else { + Assert.assertEquals( + new Interval(DateTimes.MIN, this.referenceTime.minus(period)), + rule.getEligibleInterval(this.referenceTime) + ); + } + } + + @Test + public void testForeverDropEligibleInterval() + { + final ForeverDropRule rule = new ForeverDropRule(); + Assert.assertEquals(Intervals.ETERNITY, rule.getEligibleInterval(this.referenceTime)); + } + + @Test + public void testPeriodBroadcastEligibleInterval() + { + final Period period = new Period("P15Y"); + final PeriodBroadcastDistributionRule rule = new PeriodBroadcastDistributionRule(period, true); + Assert.assertEquals( + new Interval(referenceTime.minus(period), DateTimes.MAX), + rule.getEligibleInterval(referenceTime) + ); + } + + @Test + public void testPeriodBroadcastExcludingFutureEligibleInterval() + { + final Period period = new Period("P15Y"); + final PeriodBroadcastDistributionRule rule = new PeriodBroadcastDistributionRule(period, false); + Assert.assertEquals(new Interval(period, this.referenceTime), rule.getEligibleInterval(this.referenceTime)); + } + + @Test + public void testForeverBroadcastEligibleInterval() + { + final ForeverBroadcastDistributionRule rule = new ForeverBroadcastDistributionRule(); + Assert.assertEquals(Intervals.ETERNITY, rule.getEligibleInterval(this.referenceTime)); + } + + @Test + public void testIntervalBroadcastEligibleInterval() + { + final Interval interval = Intervals.of("1993/2070"); + final IntervalBroadcastDistributionRule rule = new IntervalBroadcastDistributionRule(interval); + Assert.assertEquals(interval, rule.getEligibleInterval(this.referenceTime)); + } +} diff --git a/server/src/test/java/org/apache/druid/server/coordinator/rules/RulesTest.java b/server/src/test/java/org/apache/druid/server/coordinator/rules/RulesTest.java new file mode 100644 index 000000000000..b6bde979a302 --- /dev/null +++ b/server/src/test/java/org/apache/druid/server/coordinator/rules/RulesTest.java @@ -0,0 +1,149 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.server.coordinator.rules; + +import org.apache.druid.error.DruidExceptionMatcher; +import org.apache.druid.java.util.common.Intervals; +import org.apache.druid.java.util.common.StringUtils; +import org.joda.time.Period; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +@RunWith(Parameterized.class) +public class RulesTest +{ + // Load rules + private static final PeriodLoadRule LOAD_PT1H = new PeriodLoadRule( + new Period("PT1H"), true, null, null + ); + private static final PeriodLoadRule LOAD_PT1H_EXLUDE_FUTURE = new PeriodLoadRule( + new Period("PT1H"), false, null, null + ); + private static final PeriodLoadRule LOAD_P3M = new PeriodLoadRule( + new Period("P3M"), true, null, null + ); + private static final IntervalLoadRule LOAD_2020_2023 = new IntervalLoadRule( + Intervals.of("2020/2023"), null, null + ); + private static final IntervalLoadRule LOAD_2021_2022 = new IntervalLoadRule( + Intervals.of("2021/2022"), null, null + ); + private static final IntervalLoadRule LOAD_1980_2050 = new IntervalLoadRule( + Intervals.of("1980/2050"), null, null + ); + private static final ForeverLoadRule LOAD_FOREVER = new ForeverLoadRule(null, null); + + // Drop rules + private static final PeriodDropBeforeRule DROP_BEFORE_P3M = new PeriodDropBeforeRule(new Period("P3M")); + private static final PeriodDropBeforeRule DROP_BEFORE_P6M = new PeriodDropBeforeRule(new Period("P6M")); + private static final PeriodDropRule DROP_P1M = new PeriodDropRule(new Period("P1M"), true); + private static final PeriodDropRule DROP_P2M = new PeriodDropRule(new Period("P2M"), true); + private static final IntervalDropRule DROP_2000_2020 = new IntervalDropRule(Intervals.of("2000/2020")); + private static final IntervalDropRule DROP_2010_2020 = new IntervalDropRule(Intervals.of("2010/2020")); + private static final ForeverDropRule DROP_FOREVER = new ForeverDropRule(); + + // Broadcast rules + private static final PeriodBroadcastDistributionRule BROADCAST_PT1H = new PeriodBroadcastDistributionRule( + new Period("PT1H"), true + ); + private static final PeriodBroadcastDistributionRule BROADCAST_PT1H_EXCLUDE_FUTURE = new PeriodBroadcastDistributionRule( + new Period("PT1H"), false + ); + private static final PeriodBroadcastDistributionRule BROADCAST_PT2H = new PeriodBroadcastDistributionRule( + new Period("PT2H"), true + ); + private static final IntervalBroadcastDistributionRule BROADCAST_2000_2050 = new IntervalBroadcastDistributionRule( + Intervals.of("2000/2050") + ); + private static final IntervalBroadcastDistributionRule BROADCAST_2010_2020 = new IntervalBroadcastDistributionRule( + Intervals.of("2010/2020") + ); + private static final ForeverBroadcastDistributionRule BROADCAST_FOREVER = new ForeverBroadcastDistributionRule(); + + private final List rules; + private final boolean isInvalid; + private final Rule invalidRule1; + private final Rule invalidRule2; + + public RulesTest(final List rules, final boolean isInvalid, final Rule invalidRule1, final Rule invalidRule2) + { + this.rules = rules; + this.isInvalid = isInvalid; + this.invalidRule1 = invalidRule1; + this.invalidRule2 = invalidRule2; + } + + @Parameterized.Parameters + public static Object[] inputsAndExpectations() + { + return new Object[][] { + // Invalid rules + {getRules(LOAD_PT1H, LOAD_2021_2022, LOAD_FOREVER, LOAD_P3M), true, LOAD_FOREVER, LOAD_P3M}, + {getRules(LOAD_PT1H, LOAD_P3M, LOAD_PT1H_EXLUDE_FUTURE, LOAD_1980_2050), true, LOAD_PT1H, LOAD_PT1H_EXLUDE_FUTURE}, + {getRules(LOAD_PT1H, LOAD_P3M, LOAD_2020_2023, LOAD_P3M), true, LOAD_P3M, LOAD_P3M}, + {getRules(LOAD_2020_2023, LOAD_2021_2022, LOAD_P3M, LOAD_FOREVER), true, LOAD_2020_2023, LOAD_2021_2022}, + {getRules(LOAD_P3M, LOAD_2021_2022, LOAD_PT1H, LOAD_2020_2023), true, LOAD_P3M, LOAD_PT1H}, + {getRules(LOAD_P3M, LOAD_2021_2022, LOAD_FOREVER, LOAD_2020_2023), true, LOAD_FOREVER, LOAD_2020_2023}, + {getRules(DROP_BEFORE_P3M, DROP_P1M, DROP_BEFORE_P6M, DROP_FOREVER), true, DROP_BEFORE_P3M, DROP_BEFORE_P6M}, + {getRules(DROP_P2M, DROP_P1M, DROP_FOREVER), true, DROP_P2M, DROP_P1M}, + {getRules(DROP_2000_2020, DROP_P1M, DROP_P2M, DROP_2010_2020), true, DROP_2000_2020, DROP_2010_2020}, + {getRules(DROP_P1M, DROP_FOREVER, DROP_P2M), true, DROP_FOREVER, DROP_P2M}, + {getRules(BROADCAST_2000_2050, BROADCAST_PT1H, BROADCAST_2010_2020, BROADCAST_FOREVER), true, BROADCAST_2000_2050, BROADCAST_2010_2020}, + {getRules(BROADCAST_PT2H, BROADCAST_2000_2050, BROADCAST_PT1H, BROADCAST_FOREVER), true, BROADCAST_PT2H, BROADCAST_PT1H}, + {getRules(BROADCAST_PT1H, BROADCAST_PT1H_EXCLUDE_FUTURE, BROADCAST_FOREVER), true, BROADCAST_PT1H, BROADCAST_PT1H_EXCLUDE_FUTURE}, + {getRules(BROADCAST_PT1H, BROADCAST_PT2H, BROADCAST_2010_2020, BROADCAST_FOREVER, BROADCAST_2000_2050), true, BROADCAST_FOREVER, BROADCAST_2000_2050}, + {getRules(LOAD_PT1H, LOAD_1980_2050, LOAD_P3M, LOAD_FOREVER, DROP_FOREVER), true, LOAD_FOREVER, DROP_FOREVER}, + + // Valid rules + {null, false, null, null}, + {getRules(), false, null, null}, + {getRules(LOAD_FOREVER), false, null, null}, + {getRules(LOAD_PT1H_EXLUDE_FUTURE, LOAD_PT1H, LOAD_P3M, LOAD_FOREVER), false, null, null}, + {getRules(DROP_2010_2020, DROP_2000_2020, DROP_P1M, DROP_P2M, DROP_BEFORE_P3M, DROP_FOREVER), false, null, null}, + {getRules(BROADCAST_PT1H, BROADCAST_PT2H, BROADCAST_2010_2020, BROADCAST_2000_2050, BROADCAST_FOREVER), false, null, null}, + {getRules(DROP_BEFORE_P6M, DROP_P1M, DROP_BEFORE_P3M, LOAD_2020_2023, DROP_FOREVER), false, null, null}, + {getRules(DROP_2000_2020, LOAD_PT1H, BROADCAST_2000_2050, LOAD_1980_2050, LOAD_P3M, LOAD_FOREVER), false, null, null}, + }; + } + + private static ArrayList getRules(Rule... rules) + { + return new ArrayList<>(Arrays.asList(rules)); + } + + @Test + public void testValidateRules() + { + if (this.isInvalid) { + DruidExceptionMatcher.invalidInput().expectMessageContains( + StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", + invalidRule1, invalidRule2 + ) + ).assertThrowsAndMatches(() -> Rules.validateRules(rules)); + } else { + Rules.validateRules(rules); + } + } +} diff --git a/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java b/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java index 7ec3b5acc351..16e394ccc3c3 100644 --- a/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java +++ b/server/src/test/java/org/apache/druid/server/http/RulesResourceTest.java @@ -28,12 +28,8 @@ import org.apache.druid.java.util.common.Intervals; import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.metadata.MetadataRuleManager; -import org.apache.druid.server.coordinator.rules.ForeverBroadcastDistributionRule; -import org.apache.druid.server.coordinator.rules.ForeverDropRule; import org.apache.druid.server.coordinator.rules.ForeverLoadRule; -import org.apache.druid.server.coordinator.rules.IntervalBroadcastDistributionRule; import org.apache.druid.server.coordinator.rules.IntervalDropRule; -import org.apache.druid.server.coordinator.rules.IntervalLoadRule; import org.apache.druid.server.coordinator.rules.PeriodBroadcastDistributionRule; import org.apache.druid.server.coordinator.rules.PeriodDropBeforeRule; import org.apache.druid.server.coordinator.rules.PeriodDropRule; @@ -57,6 +53,17 @@ public class RulesResourceTest private MetadataRuleManager databaseRuleManager; private AuditManager auditManager; + private static final PeriodLoadRule LOAD_P3M = new PeriodLoadRule( + new Period("P3M"), true, null, null + ); + private final ForeverLoadRule LOAD_FOREVER = new ForeverLoadRule(null, null); + private static final PeriodDropRule DROP_P2M = new PeriodDropRule(new Period("P2M"), true); + private static final PeriodDropBeforeRule DROP_BEFORE_P6M = new PeriodDropBeforeRule(new Period("P6M")); + private static final IntervalDropRule DROP_2010_2020 = new IntervalDropRule(Intervals.of("2010/2020")); + private static final PeriodBroadcastDistributionRule BROADCAST_PT2H = new PeriodBroadcastDistributionRule( + new Period("PT2H"), true + ); + @Before public void setUp() { @@ -184,344 +191,47 @@ public void testSetDatasourceRulesWithEffectivelyNoRule() } @Test - public void testSetDatasourceRulesWithInvalidLoadRules() - { - EasyMock.replay(auditManager); - - final RulesResource rulesResource = new RulesResource(databaseRuleManager, auditManager); - final HttpServletRequest req = EasyMock.createMock(HttpServletRequest.class); - - final PeriodLoadRule loadPT1H = new PeriodLoadRule(new Period("PT1H"), true, null, null); - final PeriodLoadRule loadPT1HNoFuture = new PeriodLoadRule(new Period("PT1H"), false, null, null); - final PeriodLoadRule loadP3M = new PeriodLoadRule(new Period("P3M"), true, null, null); - final PeriodLoadRule loadP6M = new PeriodLoadRule(new Period("P6M"), true, null, null); - final IntervalLoadRule loadInterval2020To2023 = new IntervalLoadRule( - Intervals.of("2020/2023"), - null, - null - ); - final IntervalLoadRule loadInterval2021To2022 = new IntervalLoadRule( - Intervals.of("2021/2022"), - null, - null - ); - final ForeverLoadRule loadForever = new ForeverLoadRule(null, null); - - final List rules = new ArrayList<>(); - rules.add(loadP6M); - rules.add(loadP3M); - rules.add(loadForever); - - DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", loadP6M, loadP3M) - ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); - - rules.clear(); - rules.add(loadP3M); - rules.add(loadForever); - rules.add(loadP6M); - - DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", loadForever, loadP6M) - ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); - - rules.clear(); - rules.add(loadForever); - rules.add(loadPT1H); - rules.add(loadInterval2021To2022); - rules.add(loadP6M); - - DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", loadForever, loadPT1H) - ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); - - rules.clear(); - rules.add(loadPT1H); - rules.add(loadPT1HNoFuture); - rules.add(loadP6M); - - DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", loadPT1H, loadPT1HNoFuture) - ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); - - rules.clear(); - rules.add(loadPT1H); - rules.add(loadPT1H); - rules.add(loadP6M); - - DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", loadPT1H, loadPT1H) - ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); - - rules.clear(); - rules.add(loadInterval2020To2023); - rules.add(loadInterval2021To2022); - rules.add(loadP6M); - rules.add(loadForever); - - DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", loadInterval2020To2023, loadInterval2021To2022) - ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); - - rules.clear(); - rules.add(loadP6M); - rules.add(loadInterval2021To2022); - rules.add(loadP3M); - rules.add(loadInterval2020To2023); - - DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", loadP6M, loadP3M) - ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); - - - rules.clear(); - rules.add(loadP6M); - rules.add(loadInterval2020To2023); - rules.add(loadForever); - rules.add(loadInterval2021To2022); - - DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", loadInterval2020To2023, loadInterval2021To2022) - ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); - } - - @Test - public void testDatasourceRulesWithInvalidDropRules() - { - EasyMock.replay(auditManager); - - final RulesResource rulesResource = new RulesResource(databaseRuleManager, auditManager); - final HttpServletRequest req = EasyMock.createMock(HttpServletRequest.class); - - final PeriodDropBeforeRule dropBeforeByP3M = new PeriodDropBeforeRule(new Period("P3M")); - final PeriodDropBeforeRule dropBeforeByP6M = new PeriodDropBeforeRule(new Period("P6M")); - final PeriodDropRule dropByP1M = new PeriodDropRule(new Period("P1M"), true); - final IntervalDropRule dropInterval2000To2020 = new IntervalDropRule(Intervals.of("2000/2020")); - final IntervalDropRule dropInterval2010To2020 = new IntervalDropRule(Intervals.of("2010/2020")); - final PeriodDropRule dropByP1MNoFuture = new PeriodDropRule(new Period("P1M"), false); - final PeriodDropRule dropByP2M = new PeriodDropRule(new Period("P2M"), true); - final ForeverDropRule dropForever = new ForeverDropRule(); - - final List rules = new ArrayList<>(); - rules.add(dropBeforeByP3M); - rules.add(dropBeforeByP6M); - rules.add(dropByP1M); - rules.add(dropForever); - - DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", dropBeforeByP3M, dropBeforeByP6M) - ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); - - rules.clear(); - rules.add(dropByP2M); - rules.add(dropByP1M); - rules.add(dropForever); - - DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", dropByP2M, dropByP1M) - ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); - - rules.clear(); - rules.add(dropInterval2000To2020); - rules.add(dropByP1M); - rules.add(dropByP2M); - rules.add(dropInterval2010To2020); - - DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", dropInterval2000To2020, dropInterval2010To2020) - ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); - - rules.clear(); - rules.add(dropByP1M); - rules.add(dropByP1MNoFuture); - rules.add(dropForever); - - DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", dropByP1M, dropByP1MNoFuture) - ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); - - - rules.clear(); - rules.add(dropForever); - rules.add(dropByP1M); - rules.add(dropByP2M); - - DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", dropForever, dropByP1M) - ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); - } - - @Test - public void testDatasourceRulesWithInvalidBroadcastRules() + public void testSetDatasourceRulesWithValidRules() { - EasyMock.replay(auditManager); - + EasyMock.expect(databaseRuleManager.overrideRule(EasyMock.anyObject(), EasyMock.anyObject(), EasyMock.anyObject())) + .andReturn(true).anyTimes(); + EasyMock.replay(databaseRuleManager); final RulesResource rulesResource = new RulesResource(databaseRuleManager, auditManager); - final HttpServletRequest req = EasyMock.createMock(HttpServletRequest.class); - - final ForeverBroadcastDistributionRule broadcastForever = new ForeverBroadcastDistributionRule(); - final PeriodBroadcastDistributionRule broadcastPT1H = new PeriodBroadcastDistributionRule(new Period("PT1H"), true); - final PeriodBroadcastDistributionRule broadcastPT1HNoFuture = new PeriodBroadcastDistributionRule(new Period("PT1H"), false); - final PeriodBroadcastDistributionRule broadcastPT2H = new PeriodBroadcastDistributionRule(new Period("PT2H"), true); - final IntervalBroadcastDistributionRule broadcastInterval2000To2050 = new IntervalBroadcastDistributionRule( - Intervals.of("2000/2050") - ); - final IntervalBroadcastDistributionRule broadcastInterval2010To2020 = new IntervalBroadcastDistributionRule( - Intervals.of("2010/2020") - ); final List rules = new ArrayList<>(); - rules.add(broadcastInterval2000To2050); - rules.add(broadcastInterval2010To2020); - rules.add(broadcastForever); - - DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", broadcastInterval2000To2050, broadcastInterval2010To2020) - ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); - - rules.clear(); - rules.add(broadcastPT2H); - rules.add(broadcastPT1H); - rules.add(broadcastForever); - - DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", broadcastPT2H, broadcastPT1H) - ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); - - rules.clear(); - rules.add(broadcastPT1H); - rules.add(broadcastPT1H); - rules.add(broadcastForever); - - DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", broadcastPT1H, broadcastPT1H) - ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); - - // Same interval with and without future. - rules.clear(); - rules.add(broadcastPT1H); - rules.add(broadcastPT1HNoFuture); - rules.add(broadcastForever); + rules.add(BROADCAST_PT2H); + rules.add(DROP_P2M); + rules.add(DROP_2010_2020); + rules.add(LOAD_P3M); + rules.add(DROP_BEFORE_P6M); + rules.add(LOAD_FOREVER); - DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", broadcastPT1H, broadcastPT1HNoFuture) - ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); - - rules.clear(); - rules.add(broadcastPT1H); - rules.add(broadcastPT2H); - rules.add(broadcastInterval2010To2020); - rules.add(broadcastForever); - rules.add(broadcastInterval2000To2050); - - DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", broadcastForever, broadcastInterval2000To2050) - ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); + final Response resp = rulesResource.setDatasourceRules("dataSource1", rules, null, null, EasyMock.createMock(HttpServletRequest.class)); + Assert.assertEquals(200, resp.getStatus()); + EasyMock.verify(databaseRuleManager); } @Test - public void testSetDatasourceRulesWithDifferentInvalidRules() + public void testSetDatasourceRulesWithInvalidRules() { EasyMock.replay(auditManager); final RulesResource rulesResource = new RulesResource(databaseRuleManager, auditManager); final HttpServletRequest req = EasyMock.createMock(HttpServletRequest.class); - final IntervalLoadRule loadInterval1980To2050 = new IntervalLoadRule( - Intervals.of("1980/2050"), - null, - null - ); - final IntervalLoadRule loadInterval2020To2025 = new IntervalLoadRule( - Intervals.of("2020/2025"), - null, - null - ); - final PeriodLoadRule loadPT1H = new PeriodLoadRule(new Period("PT1H"), null, null, null); - final PeriodLoadRule loadP3M = new PeriodLoadRule(new Period("P3M"), null, null, null); - final PeriodLoadRule loadP6M = new PeriodLoadRule(new Period("P6M"), null, null, null); - final ForeverLoadRule loadForever = new ForeverLoadRule(null, null); - final ForeverDropRule dropForever = new ForeverDropRule(); - final PeriodBroadcastDistributionRule broadcastPT15m = new PeriodBroadcastDistributionRule(new Period("PT15m"), true); - final List rules = new ArrayList<>(); - rules.add(loadInterval2020To2025); - rules.add(broadcastPT15m); - rules.add(loadInterval1980To2050); - rules.add(loadPT1H); - rules.add(loadP3M); - rules.add(loadP6M); - rules.add(loadForever); - rules.add(dropForever); + rules.add(DROP_P2M); + rules.add(LOAD_P3M); + rules.add(DROP_BEFORE_P6M); + rules.add(BROADCAST_PT2H); + rules.add(DROP_2010_2020); + rules.add(LOAD_FOREVER); DruidExceptionMatcher.invalidInput().expectMessageContains( - StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", loadForever, dropForever) + StringUtils.format("Rule[%s] has an interval that fully contains the interval for rule[%s].", DROP_P2M, BROADCAST_PT2H) ).assertThrowsAndMatches(() -> rulesResource.setDatasourceRules("dataSource1", rules, null, null, req)); } - @Test - public void testSetDatasourceRulesWithValidRules() - { - EasyMock.expect(databaseRuleManager.overrideRule(EasyMock.anyObject(), EasyMock.anyObject(), EasyMock.anyObject())) - .andReturn(true).anyTimes(); - EasyMock.replay(databaseRuleManager); - final RulesResource rulesResource = new RulesResource(databaseRuleManager, auditManager); - - final IntervalLoadRule loadLargeInteval = new IntervalLoadRule( - Intervals.of("1980-07-29T01:00:00Z/2050-12-30T01:00:00Z"), - null, - null - ); - final IntervalLoadRule loadSmallInterval = new IntervalLoadRule( - Intervals.of("2020-09-29T01:00:00Z/2025-10-30T01:00:00Z"), - null, - null - ); - final PeriodLoadRule loadPT1H = new PeriodLoadRule(new Period("PT1H"), true, null, null); - final PeriodLoadRule loadPT1HNoFuture = new PeriodLoadRule(new Period("PT1H"), false, null, null); - final PeriodLoadRule loadP3M = new PeriodLoadRule(new Period("P3M"), true, null, null); - final PeriodLoadRule loadP6M = new PeriodLoadRule(new Period("P6M"), true, null, null); - final PeriodDropRule dropByP1M = new PeriodDropRule(new Period("P1M"), true); - final PeriodDropRule dropByP1MNoFuture = new PeriodDropRule(new Period("P1M"), false); - final ForeverLoadRule loadForever = new ForeverLoadRule(null, null); - final ForeverDropRule dropForever = new ForeverDropRule(); - final PeriodBroadcastDistributionRule broadcastPT15m = new PeriodBroadcastDistributionRule(new Period("PT15m"), true); - final PeriodBroadcastDistributionRule broadcastPT15mNoFuture = new PeriodBroadcastDistributionRule(new Period("PT15m"), false); - - final List rules = new ArrayList<>(); - rules.add(loadSmallInterval); - rules.add(loadLargeInteval); - rules.add(broadcastPT15m); - rules.add(loadPT1HNoFuture); - rules.add(loadPT1H); - rules.add(dropByP1MNoFuture); - rules.add(dropByP1M); - rules.add(loadP3M); - rules.add(loadP6M); - rules.add(loadForever); - - final Response resp = rulesResource.setDatasourceRules("dataSource1", rules, null, null, EasyMock.createMock(HttpServletRequest.class)); - Assert.assertEquals(200, resp.getStatus()); - EasyMock.verify(databaseRuleManager); - - rules.clear(); - rules.add(broadcastPT15mNoFuture); - rules.add(broadcastPT15m); - rules.add(loadPT1HNoFuture); - rules.add(loadPT1H); - rules.add(dropByP1MNoFuture); - rules.add(dropByP1M); - rules.add(loadP3M); - rules.add(loadP6M); - rules.add(loadSmallInterval); - rules.add(loadLargeInteval); - rules.add(dropForever); - - final Response resp2 = rulesResource.setDatasourceRules("dataSource1", rules, null, null, EasyMock.createMock(HttpServletRequest.class)); - Assert.assertEquals(200, resp2.getStatus()); - EasyMock.verify(databaseRuleManager); - } - @Test public void testGetAllDatasourcesRuleHistoryWithCount() { From 966c902d297403d1852ab6152e78388d3f20de03 Mon Sep 17 00:00:00 2001 From: Abhishek Balaji Radhakrishnan Date: Tue, 26 Sep 2023 23:03:29 -0700 Subject: [PATCH 9/9] fixup intellij inspection and update comment. --- .../apache/druid/server/coordinator/rules/Rule.java | 4 ++-- .../apache/druid/server/coordinator/rules/Rules.java | 11 ++++++----- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/Rule.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/Rule.java index 72c02ceb5416..dfb03102fe51 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/Rule.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/Rule.java @@ -53,8 +53,8 @@ public interface Rule /** * Returns the interval eligible for this rule. The interval must be computed based on the rule type - * optionally using {@code referenceTimestamp}. {@code referenceTimestamp} must be a timestamp - * between [{@link org.apache.druid.java.util.common.DateTimes.MIN}, {@link org.apache.druid.java.util.common.DateTimes.MAX}). + * optionally using {@code referenceTimestamp}. {@code referenceTimestamp} must be a {@link DateTime} + * between [{@link org.apache.druid.java.util.common.DateTimes#MIN}, {@link org.apache.druid.java.util.common.DateTimes#MAX}]. */ Interval getEligibleInterval(DateTime referenceTimestamp); } diff --git a/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java b/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java index bf068de088c2..5dc714d4f451 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/rules/Rules.java @@ -55,14 +55,15 @@ private Rules() * is not fully covered by the eligible interval of any preceding rule in the list. *

* Consider two rules r1 and r2. Assume r1 and r2's eligible intervals at the time when the rules are evaluated are - * i1 and i2 respectively. r1 and r2 are invalid if: + * i1 and i2, respectively. r1 and r2 are invalid if: *

    *
  • i1 is eternity. i.e., eternity fully covers i2 and any other interval that follows it. Or
    • *
  • i1 fully contains i2 and
    • - *
  • r1's eligible interval at i1's start and end fully contain r2's eligible interval at i1's start and end - * respectively. This boundary check is used to identify rules that will fire at some point. i.e., period based rules - * will return distinct eligible intervals at the boundaries, whereas broadcast and interval based rules will return - * fixed intervals regardless of the boundary.
    • + *
  • r1's eligible intervals at i1's start and end fully contain r2's eligible intervals at i1's start and end, + * respectively. This boundary check is used to identify rules that will fire at some point. i.e., period type rules + * will yield distinct eligible intervals at the boundaries, whereas broadcast and interval type rules will return + * fixed intervals regardless of the boundary. Therefore, period rules cannot always fully contain interval + * rules and vice-versa.
    • *
*

* @throws org.apache.druid.error.DruidException with error code "invalidInput" if any of the given rules is not valid.