You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "adarshsanjeev (via GitHub)" <gi...@apache.org> on 2023/06/15 05:16:43 UTC
[GitHub] [druid] adarshsanjeev opened a new pull request, #14432: Allow empty tiered replicants map for load rules
adarshsanjeev opened a new pull request, #14432:
URL: https://github.com/apache/druid/pull/14432
This PR makes some changes around the allowed tiered replicants for load rules. It allows values empty tiered replicant in load rules, which would translate to the segment being marked as used, but not loaded on any historical for example:
```
{
"interval": "2019-08-25T12:00:00.000Z/2019-08-26T00:00:00.000Z",
"type": "loadByInterval"
}
```
A new validation is also added, if a tiered replicant is being specified, it should be more than 0 (instead of allowing 0 as before), as it now only makes sense to specify it if the segment needs to be loaded on a historical.
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] adarshsanjeev commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "adarshsanjeev (via GitHub)" <gi...@apache.org>.
adarshsanjeev commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1235086903
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java:
##########
@@ -35,10 +38,30 @@ public void run(DataSegment segment, SegmentActionHandler handler)
handler.replicateSegment(segment, getTieredReplicants());
}
- protected static void validateTieredReplicants(final Map<String, Integer> tieredReplicants)
+ /**
+ * Function to create a tiered replicants map choosing default values, in case the map is null.
+ * {@code useDefaultTierForNull} decides the default value.
+ * <br>
+ * If the boolean is true, the default value is a singleton map with key {@link DruidServer#DEFAULT_NUM_REPLICANTS}
+ * and value @{@link DruidServer#DEFAULT_TIER}.
+ * <br>
+ * If the boolean is false, the default value is an empty map. This will enable the new behaviour of not loading
+ * segments to a historical unless the tier and number of replicants are explicitly specified.
+ */
+ protected static Map<String, Integer> createTieredReplicants(final Map<String, Integer> tieredReplicants, boolean useDefaultTierForNull)
{
- if (tieredReplicants.size() == 0) {
- throw new IAE("A rule with empty tiered replicants is invalid");
+ if (useDefaultTierForNull) {
+ return Configs.valueOrDefault(tieredReplicants, ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS));
+ } else {
+ return Configs.valueOrDefault(tieredReplicants, ImmutableMap.of());
+ }
+ }
+
+ protected static void validateTieredReplicants(final Map<String, Integer> tieredReplicants, boolean useDefaultTierForNull)
+ {
+ if (tieredReplicants.size() == 0 && useDefaultTierForNull) {
+ // If useDefaultTierForNull is true, null is translated to a default tier, and an empty replicant tier map is not allowed.
+ throw new IAE("A rule with empty tiered replicants is invalid unless \"useDefaultTierForNull\" is set to false.");
Review Comment:
Removed this for now, I guess that this would not break backward compatibility since we are relaxing the validations.
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java:
##########
@@ -35,10 +38,30 @@ public void run(DataSegment segment, SegmentActionHandler handler)
handler.replicateSegment(segment, getTieredReplicants());
}
- protected static void validateTieredReplicants(final Map<String, Integer> tieredReplicants)
+ /**
+ * Function to create a tiered replicants map choosing default values, in case the map is null.
+ * {@code useDefaultTierForNull} decides the default value.
+ * <br>
+ * If the boolean is true, the default value is a singleton map with key {@link DruidServer#DEFAULT_NUM_REPLICANTS}
+ * and value @{@link DruidServer#DEFAULT_TIER}.
+ * <br>
+ * If the boolean is false, the default value is an empty map. This will enable the new behaviour of not loading
+ * segments to a historical unless the tier and number of replicants are explicitly specified.
+ */
+ protected static Map<String, Integer> createTieredReplicants(final Map<String, Integer> tieredReplicants, boolean useDefaultTierForNull)
{
- if (tieredReplicants.size() == 0) {
- throw new IAE("A rule with empty tiered replicants is invalid");
+ if (useDefaultTierForNull) {
+ return Configs.valueOrDefault(tieredReplicants, ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS));
+ } else {
+ return Configs.valueOrDefault(tieredReplicants, ImmutableMap.of());
+ }
+ }
+
+ protected static void validateTieredReplicants(final Map<String, Integer> tieredReplicants, boolean useDefaultTierForNull)
+ {
+ if (tieredReplicants.size() == 0 && useDefaultTierForNull) {
+ // If useDefaultTierForNull is true, null is translated to a default tier, and an empty replicant tier map is not allowed.
+ throw new IAE("A rule with empty tiered replicants is invalid unless \"useDefaultTierForNull\" is set to false.");
}
for (Map.Entry<String, Integer> entry : tieredReplicants.entrySet()) {
if (entry.getValue() == null) {
Review Comment:
Changed these exception types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz merged pull request #14432: Allow empty tiered replicants map for load rules
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz merged PR #14432:
URL: https://github.com/apache/druid/pull/14432
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] adarshsanjeev commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "adarshsanjeev (via GitHub)" <gi...@apache.org>.
adarshsanjeev commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1235084317
##########
server/src/test/java/org/apache/druid/server/coordinator/rules/PeriodLoadRuleTest.java:
##########
@@ -186,4 +192,69 @@ public void testMappingNull() throws Exception
Assert.assertEquals(expectedPeriodLoadRule.getPeriod(), inputPeriodLoadRule.getPeriod());
Assert.assertEquals(expectedPeriodLoadRule.isIncludeFuture(), inputPeriodLoadRule.isIncludeFuture());
}
+
+ @Test(expected = IAE.class)
+ public void testCreatingNegativeTieredReplicants()
+ {
+ new PeriodLoadRule(Period.days(1), true, ImmutableMap.of(DruidServer.DEFAULT_TIER, -1), true);
+ }
+
+ @Test(expected = IAE.class)
Review Comment:
Changed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1236815501
##########
docs/operations/rule-configuration.md:
##########
@@ -109,7 +109,58 @@ In the web console you can use the up and down arrows on the right side of the i
Load rules define how Druid assigns segments to [historical process tiers](./mixed-workloads.md#historical-tiering), and how many replicas of a segment exist in each tier.
-If you have a single tier, Druid automatically names the tier `_default` and loads all segments onto it. If you define an additional tier, you must define a load rule to specify which segments to load on that tier. Until you define a load rule, your new tier remains empty.
+If you have a single tier, Druid automatically names the tier `_default`. If you define an additional tier, you must define a load rule to specify which segments to load on that tier. Until you define a load rule, your new tier remains empty.
+
+`useDefaultTierForNull` is an optional parameter that can be passed to a Load Rule. This parameter determines the default value of `tieredReplicants` only has an effect if the field is not present. The default value of `useDefaultTierForNull` is true.
+
+If this parameter is true, a missing `tieredReplicant` in the load rule is assumed to mean that the segment matching this rule should be loaded on historicals by default. This will initialize `tieredReplicants` with the default tier `"tieredReplicants": { "_default_tier": 2 }`.
+
+If this parameter is false, a missing `tieredReplicants` in the load rule is assumed to mean that the segment matching this rule does not need to be loaded on any historical by default. This will mean that these segments are excluded from most queries as well. This will initialize `tieredReplicants` as an empty map.
+
+Example:
Review Comment:
Examples are not needed here, as this section is common to all load rules. You can either leave out the examples for now or just give one example in any one of the specific load rule implementations.
##########
docs/operations/rule-configuration.md:
##########
@@ -109,7 +109,58 @@ In the web console you can use the up and down arrows on the right side of the i
Load rules define how Druid assigns segments to [historical process tiers](./mixed-workloads.md#historical-tiering), and how many replicas of a segment exist in each tier.
-If you have a single tier, Druid automatically names the tier `_default` and loads all segments onto it. If you define an additional tier, you must define a load rule to specify which segments to load on that tier. Until you define a load rule, your new tier remains empty.
+If you have a single tier, Druid automatically names the tier `_default`. If you define an additional tier, you must define a load rule to specify which segments to load on that tier. Until you define a load rule, your new tier remains empty.
+
+`useDefaultTierForNull` is an optional parameter that can be passed to a Load Rule. This parameter determines the default value of `tieredReplicants` only has an effect if the field is not present. The default value of `useDefaultTierForNull` is true.
+
+If this parameter is true, a missing `tieredReplicant` in the load rule is assumed to mean that the segment matching this rule should be loaded on historicals by default. This will initialize `tieredReplicants` with the default tier `"tieredReplicants": { "_default_tier": 2 }`.
+
+If this parameter is false, a missing `tieredReplicants` in the load rule is assumed to mean that the segment matching this rule does not need to be loaded on any historical by default. This will mean that these segments are excluded from most queries as well. This will initialize `tieredReplicants` as an empty map.
Review Comment:
All load rules can have these properties:
|Property|Description|Required|Default value|
|---------|-----------|---------|-------------|
| `tieredReplicants`| Map from tier names to the respective number of segment replicas to be loaded on those tiers. The number of replicas for each tier must be either 0 or a positive integer.| No | When `useDefaultTierForNull` is `true`, the default value is `{"_default_tier": 2}` i.e. 2 replicas to be loaded on the `_default_tier`.<br/><br/>When `useDefaultTierForNull` is `false`, the default value is `{}` i.e. no replicas to be loaded on any tier. |
|`useDefaultTierForNull`|Determines the default value of `tieredReplicants` if it is not specified or set to `null`.| No | `true`|
Specific types of load rules discussed below may have other properties too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1234825217
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java:
##########
@@ -35,10 +38,30 @@ public void run(DataSegment segment, SegmentActionHandler handler)
handler.replicateSegment(segment, getTieredReplicants());
}
- protected static void validateTieredReplicants(final Map<String, Integer> tieredReplicants)
+ /**
+ * Function to create a tiered replicants map choosing default values, in case the map is null.
+ * {@code useDefaultTierForNull} decides the default value.
+ * <br>
+ * If the boolean is true, the default value is a singleton map with key {@link DruidServer#DEFAULT_NUM_REPLICANTS}
+ * and value @{@link DruidServer#DEFAULT_TIER}.
+ * <br>
+ * If the boolean is false, the default value is an empty map. This will enable the new behaviour of not loading
+ * segments to a historical unless the tier and number of replicants are explicitly specified.
+ */
+ protected static Map<String, Integer> createTieredReplicants(final Map<String, Integer> tieredReplicants, boolean useDefaultTierForNull)
{
- if (tieredReplicants.size() == 0) {
- throw new IAE("A rule with empty tiered replicants is invalid");
+ if (useDefaultTierForNull) {
+ return Configs.valueOrDefault(tieredReplicants, ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS));
+ } else {
+ return Configs.valueOrDefault(tieredReplicants, ImmutableMap.of());
+ }
+ }
+
+ protected static void validateTieredReplicants(final Map<String, Integer> tieredReplicants, boolean useDefaultTierForNull)
+ {
+ if (tieredReplicants.size() == 0 && useDefaultTierForNull) {
+ // If useDefaultTierForNull is true, null is translated to a default tier, and an empty replicant tier map is not allowed.
+ throw new IAE("A rule with empty tiered replicants is invalid unless \"useDefaultTierForNull\" is set to false.");
}
for (Map.Entry<String, Integer> entry : tieredReplicants.entrySet()) {
if (entry.getValue() == null) {
Review Comment:
Try using this exception in the next line instead:
```suggestion
throw InvalidInput.exception("Invalid number of replicas for tier [%s], must not be null", entry.getKey());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] adarshsanjeev commented on pull request #14432: Allow empty tiered replicants map for load rules
Posted by "adarshsanjeev (via GitHub)" <gi...@apache.org>.
adarshsanjeev commented on PR #14432:
URL: https://github.com/apache/druid/pull/14432#issuecomment-1592731549
I think Option 1 has the potential to be confusing. For example
```
{
"type": "loadByInterval",
"tieredReplicants": {},
"interval": "2010-01-01/2020-01-01"
}
```
and
```
{
"type": "loadByInterval",
"interval": "2010-01-01/2020-01-01"
}
```
would result in one loading the segments on historicals and one would not, which would end up being difficult to understand moving forward.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] adarshsanjeev commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "adarshsanjeev (via GitHub)" <gi...@apache.org>.
adarshsanjeev commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1235085284
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java:
##########
@@ -41,16 +41,19 @@ public class PeriodLoadRule extends LoadRule
private final Period period;
private final boolean includeFuture;
private final Map<String, Integer> tieredReplicants;
+ private final boolean useDefaultTierForNull;
@JsonCreator
public PeriodLoadRule(
@JsonProperty("period") Period period,
@JsonProperty("includeFuture") Boolean includeFuture,
- @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants
+ @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants,
+ @JsonProperty("useDefaultTierForNull") @Nullable Boolean useDefaultTierForNull
)
{
- this.tieredReplicants = tieredReplicants == null ? ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS) : tieredReplicants;
- validateTieredReplicants(this.tieredReplicants);
+ this.useDefaultTierForNull = Configs.valueOrDefault(useDefaultTierForNull, true);
+ this.tieredReplicants = createTieredReplicants(tieredReplicants, this.useDefaultTierForNull);
+ validateTieredReplicants(this.tieredReplicants, this.useDefaultTierForNull);
Review Comment:
Moved here, I've kept the Jackson the same for now, but the code repetition has reduced a lot.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1234677521
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalLoadRule.java:
##########
@@ -64,6 +67,12 @@ public Map<String, Integer> getTieredReplicants()
return tieredReplicants;
}
+ @JsonProperty("useDefaultTierForNull")
Review Comment:
You need not specify the property name here, keeping in line with the rest of the Json getters in this class.
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java:
##########
@@ -41,16 +41,19 @@ public class PeriodLoadRule extends LoadRule
private final Period period;
private final boolean includeFuture;
private final Map<String, Integer> tieredReplicants;
+ private final boolean useDefaultTierForNull;
@JsonCreator
public PeriodLoadRule(
@JsonProperty("period") Period period,
@JsonProperty("includeFuture") Boolean includeFuture,
- @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants
+ @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants,
+ @JsonProperty("useDefaultTierForNull") @Nullable Boolean useDefaultTierForNull
)
{
- this.tieredReplicants = tieredReplicants == null ? ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS) : tieredReplicants;
- validateTieredReplicants(this.tieredReplicants);
+ this.useDefaultTierForNull = Configs.valueOrDefault(useDefaultTierForNull, true);
+ this.tieredReplicants = createTieredReplicants(tieredReplicants, this.useDefaultTierForNull);
+ validateTieredReplicants(this.tieredReplicants, this.useDefaultTierForNull);
Review Comment:
I wonder if we wouldn't benefit from moving all of this to `LoadRule` itself now by adding a constructor there. Even the fields `tieredReplicants` and `useDefaultTierForNull` can move there.
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java:
##########
@@ -35,10 +38,30 @@ public void run(DataSegment segment, SegmentActionHandler handler)
handler.replicateSegment(segment, getTieredReplicants());
}
- protected static void validateTieredReplicants(final Map<String, Integer> tieredReplicants)
+ /**
+ * Function to create a tiered replicants map choosing default values, in case the map is null.
+ * {@code useDefaultTierForNull} decides the default value.
+ * <br>
+ * If the boolean is true, the default value is a singleton map with key {@link DruidServer#DEFAULT_NUM_REPLICANTS}
+ * and value @{@link DruidServer#DEFAULT_TIER}.
+ * <br>
+ * If the boolean is false, the default value is an empty map. This will enable the new behaviour of not loading
+ * segments to a historical unless the tier and number of replicants are explicitly specified.
+ */
Review Comment:
```suggestion
/**
* Returns the given {@code tieredReplicants} map unchanged if it is non-null (including empty).
* Returns the following default values if the given map is null.
* <ul>
* <li>If {@code useDefaultTierForNull} is true, returns a singleton map from {@link DruidServer#DEFAULT_TIER} to {@link DruidServer#DEFAULT_NUM_REPLICANTS}.</li>
* <li>If {@code useDefaultTierForNull} is false, returns an empty map. This causes segments to have a replication factor of 0 and not get assigned to any historical.</li>
* </ul>
*/
```
##########
server/src/test/java/org/apache/druid/server/coordinator/rules/PeriodLoadRuleTest.java:
##########
@@ -186,4 +192,69 @@ public void testMappingNull() throws Exception
Assert.assertEquals(expectedPeriodLoadRule.getPeriod(), inputPeriodLoadRule.getPeriod());
Assert.assertEquals(expectedPeriodLoadRule.isIncludeFuture(), inputPeriodLoadRule.isIncludeFuture());
}
+
+ @Test(expected = IAE.class)
+ public void testCreatingNegativeTieredReplicants()
+ {
+ new PeriodLoadRule(Period.days(1), true, ImmutableMap.of(DruidServer.DEFAULT_TIER, -1), true);
+ }
+
+ @Test(expected = IAE.class)
+ public void testEmptyReplicantValue() throws Exception
+ {
+ // Immutable map does not allow null values
+ Map<String, Integer> tieredReplicants = new HashMap<>();
+ tieredReplicants.put("tier", null);
+ PeriodLoadRule rule = new PeriodLoadRule(
+ Period.days(1),
+ true,
+ tieredReplicants,
+ true
+ );
+
+ ObjectMapper jsonMapper = new DefaultObjectMapper();
Review Comment:
Use a static class-level field instead.
##########
server/src/test/java/org/apache/druid/server/coordinator/rules/PeriodLoadRuleTest.java:
##########
@@ -186,4 +192,69 @@ public void testMappingNull() throws Exception
Assert.assertEquals(expectedPeriodLoadRule.getPeriod(), inputPeriodLoadRule.getPeriod());
Assert.assertEquals(expectedPeriodLoadRule.isIncludeFuture(), inputPeriodLoadRule.isIncludeFuture());
}
+
+ @Test(expected = IAE.class)
+ public void testCreatingNegativeTieredReplicants()
+ {
+ new PeriodLoadRule(Period.days(1), true, ImmutableMap.of(DruidServer.DEFAULT_TIER, -1), true);
+ }
+
+ @Test(expected = IAE.class)
Review Comment:
Use `Assert.assertThrows` instead and verify the exception message too.
##########
services/src/test/java/org/apache/druid/server/router/CoordinatorRuleManagerTest.java:
##########
@@ -109,7 +109,7 @@ public void testGetRulesWithKnownDatasourceReturningAllRulesWithDefaultRule()
manager.poll();
final List<Rule> rules = manager.getRulesWithDefault(DATASOURCE2);
final List<Rule> expectedRules = new ArrayList<>();
- expectedRules.add(new ForeverLoadRule(null));
+ expectedRules.add(new ForeverLoadRule(null, null));
Review Comment:
Maybe pass `true` explicitly in all tests where you want to retain the old behaviour. It would help you distinguish between tests for old and new behaviour.
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java:
##########
@@ -35,10 +38,30 @@ public void run(DataSegment segment, SegmentActionHandler handler)
handler.replicateSegment(segment, getTieredReplicants());
}
- protected static void validateTieredReplicants(final Map<String, Integer> tieredReplicants)
+ /**
+ * Function to create a tiered replicants map choosing default values, in case the map is null.
+ * {@code useDefaultTierForNull} decides the default value.
+ * <br>
+ * If the boolean is true, the default value is a singleton map with key {@link DruidServer#DEFAULT_NUM_REPLICANTS}
+ * and value @{@link DruidServer#DEFAULT_TIER}.
+ * <br>
+ * If the boolean is false, the default value is an empty map. This will enable the new behaviour of not loading
+ * segments to a historical unless the tier and number of replicants are explicitly specified.
+ */
+ protected static Map<String, Integer> createTieredReplicants(final Map<String, Integer> tieredReplicants, boolean useDefaultTierForNull)
Review Comment:
Maybe rename this to `handleNullTieredReplicants` or similar.
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java:
##########
@@ -35,10 +38,30 @@ public void run(DataSegment segment, SegmentActionHandler handler)
handler.replicateSegment(segment, getTieredReplicants());
}
- protected static void validateTieredReplicants(final Map<String, Integer> tieredReplicants)
+ /**
+ * Function to create a tiered replicants map choosing default values, in case the map is null.
+ * {@code useDefaultTierForNull} decides the default value.
+ * <br>
+ * If the boolean is true, the default value is a singleton map with key {@link DruidServer#DEFAULT_NUM_REPLICANTS}
+ * and value @{@link DruidServer#DEFAULT_TIER}.
+ * <br>
+ * If the boolean is false, the default value is an empty map. This will enable the new behaviour of not loading
+ * segments to a historical unless the tier and number of replicants are explicitly specified.
+ */
+ protected static Map<String, Integer> createTieredReplicants(final Map<String, Integer> tieredReplicants, boolean useDefaultTierForNull)
{
- if (tieredReplicants.size() == 0) {
- throw new IAE("A rule with empty tiered replicants is invalid");
+ if (useDefaultTierForNull) {
+ return Configs.valueOrDefault(tieredReplicants, ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS));
+ } else {
+ return Configs.valueOrDefault(tieredReplicants, ImmutableMap.of());
+ }
+ }
+
+ protected static void validateTieredReplicants(final Map<String, Integer> tieredReplicants, boolean useDefaultTierForNull)
+ {
+ if (tieredReplicants.size() == 0 && useDefaultTierForNull) {
+ // If useDefaultTierForNull is true, null is translated to a default tier, and an empty replicant tier map is not allowed.
+ throw new IAE("A rule with empty tiered replicants is invalid unless \"useDefaultTierForNull\" is set to false.");
Review Comment:
Do we still need this validation? If there is a valid use-case for empty tiered replicants, I think we can just allow it regardless of the value of `useDefaultTierForNull`. Also `useDefaultTierForNull` is just meant to handle null values and using it here becomes confusing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1235284217
##########
server/src/test/java/org/apache/druid/server/coordinator/rules/ForeverLoadRuleTest.java:
##########
@@ -47,45 +51,90 @@ public void testSerdeNullTieredReplicants() throws Exception
}
@Test
- public void testMappingNullTieredReplicants() throws Exception
+ public void testCreatingNegativeTieredReplicants()
{
- String inputJson = "{\n"
- + " \"type\": \"loadForever\"\n"
- + "}";
- String expectedJson = " {\n"
- + " \"tieredReplicants\": {\n"
- + " \"" + DruidServer.DEFAULT_TIER + "\": " + DruidServer.DEFAULT_NUM_REPLICANTS + "\n"
- + " },\n"
- + " \"type\": \"loadForever\"\n"
- + " }";
- ObjectMapper jsonMapper = new DefaultObjectMapper();
- ForeverLoadRule inputForeverLoadRule = jsonMapper.readValue(inputJson, ForeverLoadRule.class);
- ForeverLoadRule expectedForeverLoadRule = jsonMapper.readValue(expectedJson, ForeverLoadRule.class);
- Assert.assertEquals(expectedForeverLoadRule.getTieredReplicants(), inputForeverLoadRule.getTieredReplicants());
+ MatcherAssert.assertThat(
+ Assert.assertThrows(DruidException.class, () ->
+ new ForeverLoadRule(
+ ImmutableMap.of(DruidServer.DEFAULT_TIER, -1),
+ null
+ )
+ ),
+ DruidExceptionMatcher.invalidInput().expectMessage(
Review Comment:
Nit: Can use `expectMessageContains` instead of a `Matcher.containsString`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1235179115
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java:
##########
@@ -29,29 +33,63 @@
*/
public abstract class LoadRule implements Rule
{
+ protected final Map<String, Integer> tieredReplicants;
Review Comment:
Why do these fields need to be protected? They can remain private and the sub-classes can call the getters when needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1234825217
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java:
##########
@@ -35,10 +38,30 @@ public void run(DataSegment segment, SegmentActionHandler handler)
handler.replicateSegment(segment, getTieredReplicants());
}
- protected static void validateTieredReplicants(final Map<String, Integer> tieredReplicants)
+ /**
+ * Function to create a tiered replicants map choosing default values, in case the map is null.
+ * {@code useDefaultTierForNull} decides the default value.
+ * <br>
+ * If the boolean is true, the default value is a singleton map with key {@link DruidServer#DEFAULT_NUM_REPLICANTS}
+ * and value @{@link DruidServer#DEFAULT_TIER}.
+ * <br>
+ * If the boolean is false, the default value is an empty map. This will enable the new behaviour of not loading
+ * segments to a historical unless the tier and number of replicants are explicitly specified.
+ */
+ protected static Map<String, Integer> createTieredReplicants(final Map<String, Integer> tieredReplicants, boolean useDefaultTierForNull)
{
- if (tieredReplicants.size() == 0) {
- throw new IAE("A rule with empty tiered replicants is invalid");
+ if (useDefaultTierForNull) {
+ return Configs.valueOrDefault(tieredReplicants, ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS));
+ } else {
+ return Configs.valueOrDefault(tieredReplicants, ImmutableMap.of());
+ }
+ }
+
+ protected static void validateTieredReplicants(final Map<String, Integer> tieredReplicants, boolean useDefaultTierForNull)
+ {
+ if (tieredReplicants.size() == 0 && useDefaultTierForNull) {
+ // If useDefaultTierForNull is true, null is translated to a default tier, and an empty replicant tier map is not allowed.
+ throw new IAE("A rule with empty tiered replicants is invalid unless \"useDefaultTierForNull\" is set to false.");
}
for (Map.Entry<String, Integer> entry : tieredReplicants.entrySet()) {
if (entry.getValue() == null) {
Review Comment:
Try using this exception in the next line instead:
```suggestion
throw InvalidInput.exception("Invalid number of replicas for tier [%s]. Value must not be null.", entry.getKey());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] cryptoe commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1233829373
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverLoadRule.java:
##########
@@ -34,14 +35,28 @@
public class ForeverLoadRule extends LoadRule
{
private final Map<String, Integer> tieredReplicants;
+ private final boolean allowEmptyTieredReplicants;
@JsonCreator
public ForeverLoadRule(
- @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants
+ @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants,
+ @JsonProperty("allowEmptyTieredReplicants") Boolean allowEmptyTieredReplicants
Review Comment:
```suggestion
@JsonProperty("useDefaultTierForNull") @Nullable Boolean useDefaultTierForNull
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] github-code-scanning[bot] commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "github-code-scanning[bot] (via GitHub)" <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1230479254
##########
server/src/test/java/org/apache/druid/server/coordinator/rules/ForeverLoadRuleTest.java:
##########
@@ -43,36 +43,25 @@
Rule reread = jsonMapper.readValue(jsonMapper.writeValueAsString(rule), Rule.class);
Assert.assertEquals(rule.getTieredReplicants(), ((ForeverLoadRule) reread).getTieredReplicants());
- Assert.assertEquals(ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS), rule.getTieredReplicants());
+ Assert.assertEquals(ImmutableMap.of(), rule.getTieredReplicants());
}
- @Test
- public void testMappingNullTieredReplicants() throws Exception
+ @Test(expected = IAE.class)
+ public void testCreatingNegativeTieredReplicants()
{
- String inputJson = "{\n"
- + " \"type\": \"loadForever\"\n"
- + "}";
- String expectedJson = " {\n"
- + " \"tieredReplicants\": {\n"
- + " \"" + DruidServer.DEFAULT_TIER + "\": " + DruidServer.DEFAULT_NUM_REPLICANTS + "\n"
- + " },\n"
- + " \"type\": \"loadForever\"\n"
- + " }";
- ObjectMapper jsonMapper = new DefaultObjectMapper();
- ForeverLoadRule inputForeverLoadRule = jsonMapper.readValue(inputJson, ForeverLoadRule.class);
- ForeverLoadRule expectedForeverLoadRule = jsonMapper.readValue(expectedJson, ForeverLoadRule.class);
- Assert.assertEquals(expectedForeverLoadRule.getTieredReplicants(), inputForeverLoadRule.getTieredReplicants());
+ ForeverLoadRule foreverLoadRule = new ForeverLoadRule(ImmutableMap.of(DruidServer.DEFAULT_TIER, -1));
Review Comment:
## Unread local variable
Variable 'ForeverLoadRule foreverLoadRule' is never read.
[Show more details](https://github.com/apache/druid/security/code-scanning/5087)
##########
server/src/test/java/org/apache/druid/server/coordinator/rules/IntervalLoadRuleTest.java:
##########
@@ -56,29 +61,30 @@
Rule reread = jsonMapper.readValue(jsonMapper.writeValueAsString(rule), Rule.class);
Assert.assertEquals(rule, reread);
- Assert.assertEquals(
- ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS),
- rule.getTieredReplicants()
+ Assert.assertEquals(ImmutableMap.of(), rule.getTieredReplicants());
+ }
+
+ @Test(expected = IAE.class)
+ public void testCreatingNegativeTieredReplicants()
+ {
+ IntervalLoadRule intervalLoadRule = new IntervalLoadRule(
+ Intervals.of("0/3000"),
+ ImmutableMap.of(DruidServer.DEFAULT_TIER, -1)
);
}
- @Test
- public void testMappingNullTieredReplicants() throws Exception
+ @Test(expected = IAE.class)
+ public void testEmptyReplicantValue() throws Exception
{
- String inputJson = " {\n"
- + " \"interval\": \"0000-01-01T00:00:00.000-05:50:36/3000-01-01T00:00:00.000-06:00\",\n"
- + " \"type\": \"loadByInterval\"\n"
- + " }";
- String expectedJson = "{\n"
- + " \"interval\": \"0000-01-01T00:00:00.000-05:50:36/3000-01-01T00:00:00.000-06:00\",\n"
- + " \"tieredReplicants\": {\n"
- + " \"" + DruidServer.DEFAULT_TIER + "\": " + DruidServer.DEFAULT_NUM_REPLICANTS + "\n"
- + " },\n"
- + " \"type\": \"loadByInterval\"\n"
- + " }";
+ // Immutable map does not allow null values
+ Map<String, Integer> tieredReplicants = new HashMap<>();
+ tieredReplicants.put("tier", null);
+ IntervalLoadRule rule = new IntervalLoadRule(
+ Intervals.of("0/3000"),
+ tieredReplicants
+ );
+
ObjectMapper jsonMapper = new DefaultObjectMapper();
- IntervalLoadRule inputIntervalLoadRule = jsonMapper.readValue(inputJson, IntervalLoadRule.class);
- IntervalLoadRule expectedIntervalLoadRule = jsonMapper.readValue(expectedJson, IntervalLoadRule.class);
- Assert.assertEquals(expectedIntervalLoadRule, inputIntervalLoadRule);
+ Rule reread = jsonMapper.readValue(jsonMapper.writeValueAsString(rule), Rule.class);
Review Comment:
## Unread local variable
Variable 'Rule reread' is never read.
[Show more details](https://github.com/apache/druid/security/code-scanning/5089)
##########
server/src/test/java/org/apache/druid/server/coordinator/rules/PeriodLoadRuleTest.java:
##########
@@ -186,4 +187,26 @@
Assert.assertEquals(expectedPeriodLoadRule.getPeriod(), inputPeriodLoadRule.getPeriod());
Assert.assertEquals(expectedPeriodLoadRule.isIncludeFuture(), inputPeriodLoadRule.isIncludeFuture());
}
+
+ @Test(expected = IAE.class)
+ public void testCreatingNegativeTieredReplicants()
+ {
+ PeriodLoadRule periodLoadRule = new PeriodLoadRule(Period.days(1), true, ImmutableMap.of(DruidServer.DEFAULT_TIER, -1));
Review Comment:
## Unread local variable
Variable 'PeriodLoadRule periodLoadRule' is never read.
[Show more details](https://github.com/apache/druid/security/code-scanning/5090)
##########
server/src/test/java/org/apache/druid/server/coordinator/rules/PeriodLoadRuleTest.java:
##########
@@ -186,4 +187,26 @@
Assert.assertEquals(expectedPeriodLoadRule.getPeriod(), inputPeriodLoadRule.getPeriod());
Assert.assertEquals(expectedPeriodLoadRule.isIncludeFuture(), inputPeriodLoadRule.isIncludeFuture());
}
+
+ @Test(expected = IAE.class)
+ public void testCreatingNegativeTieredReplicants()
+ {
+ PeriodLoadRule periodLoadRule = new PeriodLoadRule(Period.days(1), true, ImmutableMap.of(DruidServer.DEFAULT_TIER, -1));
+ }
+
+ @Test(expected = IAE.class)
+ public void testEmptyReplicantValue() throws Exception
+ {
+ // Immutable map does not allow null values
+ Map<String, Integer> tieredReplicants = new HashMap<>();
+ tieredReplicants.put("tier", null);
+ PeriodLoadRule rule = new PeriodLoadRule(
+ Period.days(1),
+ true,
+ tieredReplicants
+ );
+
+ ObjectMapper jsonMapper = new DefaultObjectMapper();
+ Rule reread = jsonMapper.readValue(jsonMapper.writeValueAsString(rule), Rule.class);
Review Comment:
## Unread local variable
Variable 'Rule reread' is never read.
[Show more details](https://github.com/apache/druid/security/code-scanning/5091)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1235282745
##########
server/src/test/java/org/apache/druid/server/coordinator/rules/ForeverLoadRuleTest.java:
##########
@@ -47,45 +51,90 @@ public void testSerdeNullTieredReplicants() throws Exception
}
@Test
- public void testMappingNullTieredReplicants() throws Exception
+ public void testCreatingNegativeTieredReplicants()
{
- String inputJson = "{\n"
- + " \"type\": \"loadForever\"\n"
- + "}";
- String expectedJson = " {\n"
- + " \"tieredReplicants\": {\n"
- + " \"" + DruidServer.DEFAULT_TIER + "\": " + DruidServer.DEFAULT_NUM_REPLICANTS + "\n"
- + " },\n"
- + " \"type\": \"loadForever\"\n"
- + " }";
- ObjectMapper jsonMapper = new DefaultObjectMapper();
- ForeverLoadRule inputForeverLoadRule = jsonMapper.readValue(inputJson, ForeverLoadRule.class);
- ForeverLoadRule expectedForeverLoadRule = jsonMapper.readValue(expectedJson, ForeverLoadRule.class);
- Assert.assertEquals(expectedForeverLoadRule.getTieredReplicants(), inputForeverLoadRule.getTieredReplicants());
+ MatcherAssert.assertThat(
+ Assert.assertThrows(DruidException.class, () ->
+ new ForeverLoadRule(
+ ImmutableMap.of(DruidServer.DEFAULT_TIER, -1),
+ null
+ )
+ ),
+ DruidExceptionMatcher.invalidInput().expectMessage(
+ Matchers.containsString(
+ StringUtils.format(
+ "Invalid number of replicas for tier [%s]. Value [%d] must be positive.",
Review Comment:
Nit: In test assertions, you can use the final message with values interpolated, so that the reader knows the message that is being verified.
```
Invalid number of replicas for tier [_default_tier]. Value [-1] must be positive.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] adarshsanjeev commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "adarshsanjeev (via GitHub)" <gi...@apache.org>.
adarshsanjeev commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1236692375
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java:
##########
@@ -19,39 +19,103 @@
package org.apache.druid.server.coordinator.rules;
-import org.apache.druid.java.util.common.IAE;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
+import org.apache.druid.client.DruidServer;
+import org.apache.druid.common.config.Configs;
+import org.apache.druid.error.InvalidInput;
import org.apache.druid.timeline.DataSegment;
import java.util.Map;
+import java.util.Objects;
/**
* LoadRules indicate the number of replicants a segment should have in a given tier.
Review Comment:
Added doc to the parameter as well. I think `validateTieredReplicants` already covers it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1236834173
##########
docs/operations/rule-configuration.md:
##########
@@ -109,7 +109,58 @@ In the web console you can use the up and down arrows on the right side of the i
Load rules define how Druid assigns segments to [historical process tiers](./mixed-workloads.md#historical-tiering), and how many replicas of a segment exist in each tier.
-If you have a single tier, Druid automatically names the tier `_default` and loads all segments onto it. If you define an additional tier, you must define a load rule to specify which segments to load on that tier. Until you define a load rule, your new tier remains empty.
+If you have a single tier, Druid automatically names the tier `_default`. If you define an additional tier, you must define a load rule to specify which segments to load on that tier. Until you define a load rule, your new tier remains empty.
+
+`useDefaultTierForNull` is an optional parameter that can be passed to a Load Rule. This parameter determines the default value of `tieredReplicants` only has an effect if the field is not present. The default value of `useDefaultTierForNull` is true.
+
+If this parameter is true, a missing `tieredReplicant` in the load rule is assumed to mean that the segment matching this rule should be loaded on historicals by default. This will initialize `tieredReplicants` with the default tier `"tieredReplicants": { "_default_tier": 2 }`.
+
+If this parameter is false, a missing `tieredReplicants` in the load rule is assumed to mean that the segment matching this rule does not need to be loaded on any historical by default. This will mean that these segments are excluded from most queries as well. This will initialize `tieredReplicants` as an empty map.
Review Comment:
```suggestion
All load rules can have these properties:
|Property|Description|Required|Default value|
|---------|-----------|---------|-------------|
| `tieredReplicants`| Map from tier names to the respective number of segment replicas to be loaded on those tiers. The number of replicas for each tier must be either 0 or a positive integer.| No | When `useDefaultTierForNull` is `true`, the default value is `{"_default_tier": 2}` i.e. 2 replicas to be loaded on the `_default_tier`.<br/><br/>When `useDefaultTierForNull` is `false`, the default value is `{}` i.e. no replicas to be loaded on any tier. |
|`useDefaultTierForNull`|Determines the default value of `tieredReplicants` if it is not specified or set to `null`.| No | `true`|
Specific types of load rules discussed below may have other properties too.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1234827070
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java:
##########
@@ -35,10 +38,30 @@ public void run(DataSegment segment, SegmentActionHandler handler)
handler.replicateSegment(segment, getTieredReplicants());
}
- protected static void validateTieredReplicants(final Map<String, Integer> tieredReplicants)
+ /**
+ * Function to create a tiered replicants map choosing default values, in case the map is null.
+ * {@code useDefaultTierForNull} decides the default value.
+ * <br>
+ * If the boolean is true, the default value is a singleton map with key {@link DruidServer#DEFAULT_NUM_REPLICANTS}
+ * and value @{@link DruidServer#DEFAULT_TIER}.
+ * <br>
+ * If the boolean is false, the default value is an empty map. This will enable the new behaviour of not loading
+ * segments to a historical unless the tier and number of replicants are explicitly specified.
+ */
+ protected static Map<String, Integer> createTieredReplicants(final Map<String, Integer> tieredReplicants, boolean useDefaultTierForNull)
{
- if (tieredReplicants.size() == 0) {
- throw new IAE("A rule with empty tiered replicants is invalid");
+ if (useDefaultTierForNull) {
+ return Configs.valueOrDefault(tieredReplicants, ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS));
+ } else {
+ return Configs.valueOrDefault(tieredReplicants, ImmutableMap.of());
+ }
+ }
+
+ protected static void validateTieredReplicants(final Map<String, Integer> tieredReplicants, boolean useDefaultTierForNull)
+ {
+ if (tieredReplicants.size() == 0 && useDefaultTierForNull) {
+ // If useDefaultTierForNull is true, null is translated to a default tier, and an empty replicant tier map is not allowed.
+ throw new IAE("A rule with empty tiered replicants is invalid unless \"useDefaultTierForNull\" is set to false.");
}
for (Map.Entry<String, Integer> entry : tieredReplicants.entrySet()) {
if (entry.getValue() == null) {
Review Comment:
Similarly for the case `entry.getValue() < 0`, use this instead:
```suggestion
throw InvalidInput.exception("Number of replicas for tier [%s] must be positive. Found [%d].", entry.getKey(), entry.getValue());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1234827070
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java:
##########
@@ -35,10 +38,30 @@ public void run(DataSegment segment, SegmentActionHandler handler)
handler.replicateSegment(segment, getTieredReplicants());
}
- protected static void validateTieredReplicants(final Map<String, Integer> tieredReplicants)
+ /**
+ * Function to create a tiered replicants map choosing default values, in case the map is null.
+ * {@code useDefaultTierForNull} decides the default value.
+ * <br>
+ * If the boolean is true, the default value is a singleton map with key {@link DruidServer#DEFAULT_NUM_REPLICANTS}
+ * and value @{@link DruidServer#DEFAULT_TIER}.
+ * <br>
+ * If the boolean is false, the default value is an empty map. This will enable the new behaviour of not loading
+ * segments to a historical unless the tier and number of replicants are explicitly specified.
+ */
+ protected static Map<String, Integer> createTieredReplicants(final Map<String, Integer> tieredReplicants, boolean useDefaultTierForNull)
{
- if (tieredReplicants.size() == 0) {
- throw new IAE("A rule with empty tiered replicants is invalid");
+ if (useDefaultTierForNull) {
+ return Configs.valueOrDefault(tieredReplicants, ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS));
+ } else {
+ return Configs.valueOrDefault(tieredReplicants, ImmutableMap.of());
+ }
+ }
+
+ protected static void validateTieredReplicants(final Map<String, Integer> tieredReplicants, boolean useDefaultTierForNull)
+ {
+ if (tieredReplicants.size() == 0 && useDefaultTierForNull) {
+ // If useDefaultTierForNull is true, null is translated to a default tier, and an empty replicant tier map is not allowed.
+ throw new IAE("A rule with empty tiered replicants is invalid unless \"useDefaultTierForNull\" is set to false.");
}
for (Map.Entry<String, Integer> entry : tieredReplicants.entrySet()) {
if (entry.getValue() == null) {
Review Comment:
Similarly for the case `entry.getValue() < 0`, use this instead:
```suggestion
throw InvalidInput.exception("Invalid number of replicas for tier [%s]. Value [%d] must be positive.", entry.getKey(), entry.getValue());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] adarshsanjeev commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "adarshsanjeev (via GitHub)" <gi...@apache.org>.
adarshsanjeev commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1234944890
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java:
##########
@@ -74,7 +76,12 @@ public CoordinatorStats run(
{
try {
// get the "snapshots" of targetReplicants and currentReplicants for assignments.
- targetReplicants.putAll(getTieredReplicants());
+ Map<String, Integer> tieredReplicants = getTieredReplicants();
+ if (tieredReplicants.isEmpty()) {
+ targetReplicants.put(DruidServer.DEFAULT_TIER, DEFAULT_LOAD_RULE_NUM_REPLICANTS);
+ } else {
+ targetReplicants.putAll(getTieredReplicants());
+ }
Review Comment:
Initially, this was not possible as the Coordinator code was breaking with an empty map in a few places. I observed that the segments were being dropped even if the rule was present. However, I think thanks to your coordinator changes, this has been fixed. Reverting the changes to this function as it seem to be not needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] adarshsanjeev commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "adarshsanjeev (via GitHub)" <gi...@apache.org>.
adarshsanjeev commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1234685124
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java:
##########
@@ -41,16 +41,19 @@ public class PeriodLoadRule extends LoadRule
private final Period period;
private final boolean includeFuture;
private final Map<String, Integer> tieredReplicants;
+ private final boolean useDefaultTierForNull;
@JsonCreator
public PeriodLoadRule(
@JsonProperty("period") Period period,
@JsonProperty("includeFuture") Boolean includeFuture,
- @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants
+ @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants,
+ @JsonProperty("useDefaultTierForNull") @Nullable Boolean useDefaultTierForNull
)
{
- this.tieredReplicants = tieredReplicants == null ? ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS) : tieredReplicants;
- validateTieredReplicants(this.tieredReplicants);
+ this.useDefaultTierForNull = Configs.valueOrDefault(useDefaultTierForNull, true);
+ this.tieredReplicants = createTieredReplicants(tieredReplicants, this.useDefaultTierForNull);
+ validateTieredReplicants(this.tieredReplicants, this.useDefaultTierForNull);
Review Comment:
In master, the message is returned as a warning after submitting the load rule. This behaviour here would be the same, or did I misunderstand what you mean?
<img width="583" alt="Screenshot 2023-06-20 at 8 39 57 AM" src="https://github.com/apache/druid/assets/9818861/fd844cf2-4638-4446-864b-221231087b39">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] cryptoe commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1234682523
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java:
##########
@@ -41,16 +41,19 @@ public class PeriodLoadRule extends LoadRule
private final Period period;
private final boolean includeFuture;
private final Map<String, Integer> tieredReplicants;
+ private final boolean useDefaultTierForNull;
@JsonCreator
public PeriodLoadRule(
@JsonProperty("period") Period period,
@JsonProperty("includeFuture") Boolean includeFuture,
- @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants
+ @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants,
+ @JsonProperty("useDefaultTierForNull") @Nullable Boolean useDefaultTierForNull
)
{
- this.tieredReplicants = tieredReplicants == null ? ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS) : tieredReplicants;
- validateTieredReplicants(this.tieredReplicants);
+ this.useDefaultTierForNull = Configs.valueOrDefault(useDefaultTierForNull, true);
+ this.tieredReplicants = createTieredReplicants(tieredReplicants, this.useDefaultTierForNull);
+ validateTieredReplicants(this.tieredReplicants, this.useDefaultTierForNull);
Review Comment:
Since this is set via an API RulesResources#setDatasourceRules(). How would the exception thrown here be visible to end users.
I guess we need to send a message back in the API response.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] adarshsanjeev commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "adarshsanjeev (via GitHub)" <gi...@apache.org>.
adarshsanjeev commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1234702994
##########
services/src/test/java/org/apache/druid/server/router/CoordinatorRuleManagerTest.java:
##########
@@ -109,7 +109,7 @@ public void testGetRulesWithKnownDatasourceReturningAllRulesWithDefaultRule()
manager.poll();
final List<Rule> rules = manager.getRulesWithDefault(DATASOURCE2);
final List<Rule> expectedRules = new ArrayList<>();
- expectedRules.add(new ForeverLoadRule(null));
+ expectedRules.add(new ForeverLoadRule(null, null));
Review Comment:
Would it make more sense to use null and false to differentiate old and new tests, since that is the real test for backward compatibility?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekrb19 commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1235580517
##########
server/src/test/java/org/apache/druid/server/coordinator/rules/ForeverLoadRuleTest.java:
##########
@@ -47,45 +49,79 @@ public void testSerdeNullTieredReplicants() throws Exception
}
@Test
- public void testMappingNullTieredReplicants() throws Exception
+ public void testCreatingNegativeTieredReplicants()
{
- String inputJson = "{\n"
- + " \"type\": \"loadForever\"\n"
- + "}";
- String expectedJson = " {\n"
- + " \"tieredReplicants\": {\n"
- + " \"" + DruidServer.DEFAULT_TIER + "\": " + DruidServer.DEFAULT_NUM_REPLICANTS + "\n"
- + " },\n"
- + " \"type\": \"loadForever\"\n"
- + " }";
- ObjectMapper jsonMapper = new DefaultObjectMapper();
- ForeverLoadRule inputForeverLoadRule = jsonMapper.readValue(inputJson, ForeverLoadRule.class);
- ForeverLoadRule expectedForeverLoadRule = jsonMapper.readValue(expectedJson, ForeverLoadRule.class);
- Assert.assertEquals(expectedForeverLoadRule.getTieredReplicants(), inputForeverLoadRule.getTieredReplicants());
+ MatcherAssert.assertThat(
+ Assert.assertThrows(DruidException.class, () ->
+ new ForeverLoadRule(
+ ImmutableMap.of(DruidServer.DEFAULT_TIER, -1),
+ null
+ )
+ ),
+ DruidExceptionMatcher.invalidInput().expectMessageContains(
+ "Invalid number of replicas for tier [_default_tier]. Value [-1] must be positive."
+ )
+ );
}
- @Test(expected = IAE.class)
+ @Test
public void testEmptyTieredReplicants() throws Exception
{
- ForeverLoadRule rule = new ForeverLoadRule(
- ImmutableMap.of()
- );
+ ForeverLoadRule rule = new ForeverLoadRule(ImmutableMap.of(), false);
- ObjectMapper jsonMapper = new DefaultObjectMapper();
- Rule reread = jsonMapper.readValue(jsonMapper.writeValueAsString(rule), Rule.class);
+ LoadRule reread = (LoadRule) OBJECT_MAPPER.readValue(OBJECT_MAPPER.writeValueAsString(rule), Rule.class);
+ Assert.assertEquals(ImmutableMap.of(), reread.getTieredReplicants());
}
- @Test(expected = IAE.class)
- public void testEmptyReplicantValue() throws Exception
+ @Test
+ public void testNullReplicantValue()
{
// Immutable map does not allow null values
Map<String, Integer> tieredReplicants = new HashMap<>();
tieredReplicants.put("tier", null);
- ForeverLoadRule rule = new ForeverLoadRule(
- tieredReplicants
+
+ MatcherAssert.assertThat(
+ Assert.assertThrows(DruidException.class, () ->
+ new ForeverLoadRule(
+ tieredReplicants,
+ true
+ )
+ ),
+ DruidExceptionMatcher.invalidInput().expectMessageContains(
+ "Invalid number of replicas for tier [tier]. Value must not be null."
+ )
);
+ }
- ObjectMapper jsonMapper = new DefaultObjectMapper();
- Rule reread = jsonMapper.readValue(jsonMapper.writeValueAsString(rule), Rule.class);
+ @Test
+ public void testShouldCreateDefaultTier() throws Exception
+ {
+ String inputJson = " {\n"
+ + " \"type\": \"loadForever\"\n"
+ + " }";
+ ForeverLoadRule inputForeverLoadRule = OBJECT_MAPPER.readValue(inputJson, ForeverLoadRule.class);
+ Assert.assertEquals(ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS), inputForeverLoadRule.getTieredReplicants());
+ }
+
+ @Test
+ public void testUseDefaultTierAsTrueShouldCreateDefaultTier() throws Exception
+ {
+ String inputJson = " {\n"
+ + " \"type\": \"loadForever\"\n,"
+ + " \"useDefaultTierForNull\": \"true\"\n"
+ + " }";
+ ForeverLoadRule inputForeverLoadRule = OBJECT_MAPPER.readValue(inputJson, ForeverLoadRule.class);
+ Assert.assertEquals(ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS), inputForeverLoadRule.getTieredReplicants());
+ }
+
+ @Test
+ public void testUseDefaultTierAsFalseShouldCreateEmptyMap() throws Exception
+ {
+ String inputJson = " {\n"
+ + " \"type\": \"loadForever\"\n,"
+ + " \"useDefaultTierForNull\": \"false\"\n"
+ + " }";
Review Comment:
Thanks for these tests!
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java:
##########
@@ -19,39 +19,103 @@
package org.apache.druid.server.coordinator.rules;
-import org.apache.druid.java.util.common.IAE;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
+import org.apache.druid.client.DruidServer;
+import org.apache.druid.common.config.Configs;
+import org.apache.druid.error.InvalidInput;
import org.apache.druid.timeline.DataSegment;
import java.util.Map;
+import java.util.Objects;
/**
* LoadRules indicate the number of replicants a segment should have in a given tier.
Review Comment:
Could use a javadoc update with the new property addition.
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverLoadRule.java:
##########
@@ -21,30 +21,24 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
-import com.google.common.collect.ImmutableMap;
-import org.apache.druid.client.DruidServer;
import org.apache.druid.timeline.DataSegment;
import org.joda.time.DateTime;
import org.joda.time.Interval;
+import javax.annotation.Nullable;
import java.util.Map;
-import java.util.Objects;
/**
*/
public class ForeverLoadRule extends LoadRule
{
- private final Map<String, Integer> tieredReplicants;
-
@JsonCreator
public ForeverLoadRule(
- @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants
+ @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants,
+ @JsonProperty("useDefaultTierForNull") @Nullable Boolean useDefaultTierForNull
Review Comment:
Should we document this property along with an example where `tieredReplicants` is null: https://github.com/apache/druid/blob/master/docs/operations/rule-configuration.md
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] cryptoe commented on pull request #14432: Allow empty tiered replicants map for load rules
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on PR #14432:
URL: https://github.com/apache/druid/pull/14432#issuecomment-1594219973
We could also include a new flag called `allowNullTieredReplicants` which is defaulted to `true` and does not change the current behavior ie we set the default tier rules in case `NullTieredReplicants` is null.
If users set this to `false` the new behavior is enabled. The web-console starts setting this new flag for every request. That would solve the backward compatibility piece.
Its `Option 1` with a new flag.
cc @kfaraz @adarshsanjeev
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] adarshsanjeev commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "adarshsanjeev (via GitHub)" <gi...@apache.org>.
adarshsanjeev commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1234257843
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalLoadRule.java:
##########
@@ -38,15 +38,23 @@ public class IntervalLoadRule extends LoadRule
private final Interval interval;
private final Map<String, Integer> tieredReplicants;
+ private final boolean allowEmptyTieredReplicants;
@JsonCreator
public IntervalLoadRule(
@JsonProperty("interval") Interval interval,
- @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants
+ @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants,
+ @JsonProperty("allowEmptyTieredReplicants") Boolean allowEmptyTieredReplicants
)
{
- this.tieredReplicants = tieredReplicants == null ? ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS) : tieredReplicants;
- validateTieredReplicants(this.tieredReplicants);
+ this.allowEmptyTieredReplicants = allowEmptyTieredReplicants != null && allowEmptyTieredReplicants;
Review Comment:
Thanks! This cleaned the code a lot.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1233799714
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java:
##########
@@ -74,7 +76,12 @@ public CoordinatorStats run(
{
try {
// get the "snapshots" of targetReplicants and currentReplicants for assignments.
- targetReplicants.putAll(getTieredReplicants());
+ Map<String, Integer> tieredReplicants = getTieredReplicants();
+ if (tieredReplicants.isEmpty()) {
+ targetReplicants.put(DruidServer.DEFAULT_TIER, DEFAULT_LOAD_RULE_NUM_REPLICANTS);
+ } else {
+ targetReplicants.putAll(getTieredReplicants());
+ }
Review Comment:
This manipulation should not be done here. It should have already been done by the specific implementation of `LoadRule`.
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalLoadRule.java:
##########
@@ -38,15 +38,23 @@ public class IntervalLoadRule extends LoadRule
private final Interval interval;
private final Map<String, Integer> tieredReplicants;
+ private final boolean allowEmptyTieredReplicants;
@JsonCreator
public IntervalLoadRule(
@JsonProperty("interval") Interval interval,
- @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants
+ @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants,
+ @JsonProperty("allowEmptyTieredReplicants") Boolean allowEmptyTieredReplicants
)
{
- this.tieredReplicants = tieredReplicants == null ? ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS) : tieredReplicants;
- validateTieredReplicants(this.tieredReplicants);
+ this.allowEmptyTieredReplicants = allowEmptyTieredReplicants != null && allowEmptyTieredReplicants;
Review Comment:
You can use the utility method:
```suggestion
this.useDefaultTierForNull = Configs.valueOrDefault(useDefaultTierForNull, true);
```
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalLoadRule.java:
##########
@@ -38,15 +38,23 @@ public class IntervalLoadRule extends LoadRule
private final Interval interval;
private final Map<String, Integer> tieredReplicants;
+ private final boolean allowEmptyTieredReplicants;
@JsonCreator
public IntervalLoadRule(
@JsonProperty("interval") Interval interval,
- @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants
+ @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants,
+ @JsonProperty("allowEmptyTieredReplicants") Boolean allowEmptyTieredReplicants
)
{
- this.tieredReplicants = tieredReplicants == null ? ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS) : tieredReplicants;
- validateTieredReplicants(this.tieredReplicants);
+ this.allowEmptyTieredReplicants = allowEmptyTieredReplicants != null && allowEmptyTieredReplicants;
+
+ if (!this.allowEmptyTieredReplicants) {
+ this.tieredReplicants = tieredReplicants == null ? ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS) : tieredReplicants;
+ } else {
+ this.tieredReplicants = tieredReplicants == null ? ImmutableMap.of() : tieredReplicants;
+ }
+ validateTieredReplicants(this.tieredReplicants, this.allowEmptyTieredReplicants);
Review Comment:
This code has been repeated in 3 places. It should be commoned out into `LoadRule`.
You can update the `validateTieredReplicants` to `validateAndGetTieredReplicants`.
```suggestion
this.tieredReplicants = validateAndGetTieredReplicants(tieredReplicants, this.useDefaultTierForNull);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] cryptoe commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1234740652
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java:
##########
@@ -41,16 +41,19 @@ public class PeriodLoadRule extends LoadRule
private final Period period;
private final boolean includeFuture;
private final Map<String, Integer> tieredReplicants;
+ private final boolean useDefaultTierForNull;
@JsonCreator
public PeriodLoadRule(
@JsonProperty("period") Period period,
@JsonProperty("includeFuture") Boolean includeFuture,
- @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants
+ @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants,
+ @JsonProperty("useDefaultTierForNull") @Nullable Boolean useDefaultTierForNull
)
{
- this.tieredReplicants = tieredReplicants == null ? ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS) : tieredReplicants;
- validateTieredReplicants(this.tieredReplicants);
+ this.useDefaultTierForNull = Configs.valueOrDefault(useDefaultTierForNull, true);
+ this.tieredReplicants = createTieredReplicants(tieredReplicants, this.useDefaultTierForNull);
+ validateTieredReplicants(this.tieredReplicants, this.useDefaultTierForNull);
Review Comment:
Sure this looks good to me. We can share this screen shot as part of the PR description.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1234825217
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java:
##########
@@ -35,10 +38,30 @@ public void run(DataSegment segment, SegmentActionHandler handler)
handler.replicateSegment(segment, getTieredReplicants());
}
- protected static void validateTieredReplicants(final Map<String, Integer> tieredReplicants)
+ /**
+ * Function to create a tiered replicants map choosing default values, in case the map is null.
+ * {@code useDefaultTierForNull} decides the default value.
+ * <br>
+ * If the boolean is true, the default value is a singleton map with key {@link DruidServer#DEFAULT_NUM_REPLICANTS}
+ * and value @{@link DruidServer#DEFAULT_TIER}.
+ * <br>
+ * If the boolean is false, the default value is an empty map. This will enable the new behaviour of not loading
+ * segments to a historical unless the tier and number of replicants are explicitly specified.
+ */
+ protected static Map<String, Integer> createTieredReplicants(final Map<String, Integer> tieredReplicants, boolean useDefaultTierForNull)
{
- if (tieredReplicants.size() == 0) {
- throw new IAE("A rule with empty tiered replicants is invalid");
+ if (useDefaultTierForNull) {
+ return Configs.valueOrDefault(tieredReplicants, ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS));
+ } else {
+ return Configs.valueOrDefault(tieredReplicants, ImmutableMap.of());
+ }
+ }
+
+ protected static void validateTieredReplicants(final Map<String, Integer> tieredReplicants, boolean useDefaultTierForNull)
+ {
+ if (tieredReplicants.size() == 0 && useDefaultTierForNull) {
+ // If useDefaultTierForNull is true, null is translated to a default tier, and an empty replicant tier map is not allowed.
+ throw new IAE("A rule with empty tiered replicants is invalid unless \"useDefaultTierForNull\" is set to false.");
}
for (Map.Entry<String, Integer> entry : tieredReplicants.entrySet()) {
if (entry.getValue() == null) {
Review Comment:
Try using this exception in the next line instead:
```suggestion
throw InvalidInput.exception("Number of replicas in tier [%s] is null", entry.getKey());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] adarshsanjeev commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "adarshsanjeev (via GitHub)" <gi...@apache.org>.
adarshsanjeev commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1234258353
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalLoadRule.java:
##########
@@ -38,15 +38,23 @@ public class IntervalLoadRule extends LoadRule
private final Interval interval;
private final Map<String, Integer> tieredReplicants;
+ private final boolean allowEmptyTieredReplicants;
@JsonCreator
public IntervalLoadRule(
@JsonProperty("interval") Interval interval,
- @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants
+ @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants,
+ @JsonProperty("allowEmptyTieredReplicants") Boolean allowEmptyTieredReplicants
)
{
- this.tieredReplicants = tieredReplicants == null ? ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS) : tieredReplicants;
- validateTieredReplicants(this.tieredReplicants);
+ this.allowEmptyTieredReplicants = allowEmptyTieredReplicants != null && allowEmptyTieredReplicants;
+
+ if (!this.allowEmptyTieredReplicants) {
+ this.tieredReplicants = tieredReplicants == null ? ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS) : tieredReplicants;
+ } else {
+ this.tieredReplicants = tieredReplicants == null ? ImmutableMap.of() : tieredReplicants;
+ }
+ validateTieredReplicants(this.tieredReplicants, this.allowEmptyTieredReplicants);
Review Comment:
Refactored this into a separate function instead, since `validateAndGetTieredReplicants` was starting to do two different things. Please let me know if this looks good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1234822567
##########
services/src/test/java/org/apache/druid/server/router/CoordinatorRuleManagerTest.java:
##########
@@ -109,7 +109,7 @@ public void testGetRulesWithKnownDatasourceReturningAllRulesWithDefaultRule()
manager.poll();
final List<Rule> rules = manager.getRulesWithDefault(DATASOURCE2);
final List<Rule> expectedRules = new ArrayList<>();
- expectedRules.add(new ForeverLoadRule(null));
+ expectedRules.add(new ForeverLoadRule(null, null));
Review Comment:
Sure, that works too as long as there are at least a couple of tests for each of the 3 flavors: null (for backward compat), true and false.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1235180500
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java:
##########
@@ -29,29 +33,63 @@
*/
public abstract class LoadRule implements Rule
{
+ protected final Map<String, Integer> tieredReplicants;
+ protected final boolean useDefaultTierForNull;
+
+ protected LoadRule(Map<String, Integer> tieredReplicants, Boolean useDefaultTierForNull)
+ {
+ this.useDefaultTierForNull = Configs.valueOrDefault(useDefaultTierForNull, true);
+ this.tieredReplicants = handleNullTieredReplicants(tieredReplicants, this.useDefaultTierForNull);
+ validateTieredReplicants(this.tieredReplicants);
+ }
+
+ @JsonProperty
+ public Map<String, Integer> getTieredReplicants()
+ {
+ return tieredReplicants;
+ }
+
+ @JsonProperty
+ public boolean useDefaultTierForNull()
+ {
+ return useDefaultTierForNull;
+ }
+
@Override
public void run(DataSegment segment, SegmentActionHandler handler)
{
handler.replicateSegment(segment, getTieredReplicants());
}
- protected static void validateTieredReplicants(final Map<String, Integer> tieredReplicants)
+ /**
+ * Returns the given {@code tieredReplicants} map unchanged if it is non-null (including empty).
+ * Returns the following default values if the given map is null.
+ * <ul>
+ * <li>If {@code useDefaultTierForNull} is true, returns a singleton map from {@link DruidServer#DEFAULT_TIER} to {@link DruidServer#DEFAULT_NUM_REPLICANTS}.</li>
+ * <li>If {@code useDefaultTierForNull} is false, returns an empty map. This causes segments to have a replication factor of 0 and not get assigned to any historical.</li>
+ * </ul>
+ */
+ protected static Map<String, Integer> handleNullTieredReplicants(final Map<String, Integer> tieredReplicants, boolean useDefaultTierForNull)
Review Comment:
I think both these methods `handleNull` and `validate` can be private now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #14432: Allow empty tiered replicants map for load rules
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #14432:
URL: https://github.com/apache/druid/pull/14432#discussion_r1235182097
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java:
##########
@@ -74,13 +72,6 @@ public boolean isIncludeFuture()
return includeFuture;
}
- @Override
- @JsonProperty
- public Map<String, Integer> getTieredReplicants()
- {
- return tieredReplicants;
- }
-
@Override
public int getNumReplicants(String tier)
Review Comment:
This method is also common across the 3 load rule implementations, could be moved into `LoadRule`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org