capistrant commented on code in PR #18136:
URL: https://github.com/apache/druid/pull/18136#discussion_r2176088357
##########
docs/configuration/index.md:
##########
@@ -1791,6 +1791,8 @@ These Broker configurations can be defined in the
`broker/runtime.properties` fi
|`druid.broker.balancer.type`|`random`, `connectionCount`|Determines how the
broker balances connections to Historical processes. `random` choose randomly,
`connectionCount` picks the process with the fewest number of active
connections to|`random`|
|`druid.broker.select.tier`|`highestPriority`, `lowestPriority`, `custom`|If
segments are cross-replicated across tiers in a cluster, you can tell the
broker to prefer to select segments in a tier with a certain
priority.|`highestPriority`|
Review Comment:
```suggestion
|`druid.broker.select.tier`|`highestPriority`, `lowestPriority`, `custom`,
`preferred`|If segments are cross-replicated across tiers in a cluster, you can
tell the broker to prefer to select segments in a tier with a certain
priority.|`highestPriority`|
```
##########
docs/configuration/index.md:
##########
@@ -1791,6 +1791,8 @@ These Broker configurations can be defined in the
`broker/runtime.properties` fi
|`druid.broker.balancer.type`|`random`, `connectionCount`|Determines how the
broker balances connections to Historical processes. `random` choose randomly,
`connectionCount` picks the process with the fewest number of active
connections to|`random`|
|`druid.broker.select.tier`|`highestPriority`, `lowestPriority`, `custom`|If
segments are cross-replicated across tiers in a cluster, you can tell the
broker to prefer to select segments in a tier with a certain
priority.|`highestPriority`|
|`druid.broker.select.tier.custom.priorities`|An array of integer priorities,
such as `[-1, 0, 1, 2]`|Select servers in tiers with a custom priority
list.|The config only has effect if `druid.broker.select.tier` is set to
`custom`. If `druid.broker.select.tier` is set to `custom` but this config is
not specified, the effect is the same as `druid.broker.select.tier` set to
`highestPriority`. Any of the integers in this config can be ignored if there's
no corresponding tiers with such priorities. Tiers with priorities explicitly
specified in this config always have higher priority than those not and those
not specified fall back to use `highestPriority` strategy among themselves.|
+|`druid.broker.select.tier.preferred.tier`| The preferred tier name. E.g.,
`_default_tier` | A non-empty value that specifies the preferred tier in which
historical servers will be picked up for queries. If there are no enough
historical servers from the preferred tier, servers from other tiers (if there
are) will be selected. | null |
+|`druid.broker.select.tier.preferred.priority`| `highest`, `lowest` |
Optional. If there are multiple candidates in a preferred tier, specifies the
priority to pick up candidates. By default, the higher priority a historical,
the higher chances it will be picked up. | `highest` |
Review Comment:
```suggestion
|`druid.broker.select.tier.preferred.priority`| `highest`, `lowest` | If
there are multiple candidates in a preferred tier, specifies the priority to
pick up candidates. By default, the higher priority a historical, the higher
chances it will be picked up. This config only has effect if
`druid.broker.select.tier` is set to `preferred`| `highest` |
```
##########
docs/configuration/index.md:
##########
@@ -1791,6 +1791,8 @@ These Broker configurations can be defined in the
`broker/runtime.properties` fi
|`druid.broker.balancer.type`|`random`, `connectionCount`|Determines how the
broker balances connections to Historical processes. `random` choose randomly,
`connectionCount` picks the process with the fewest number of active
connections to|`random`|
|`druid.broker.select.tier`|`highestPriority`, `lowestPriority`, `custom`|If
segments are cross-replicated across tiers in a cluster, you can tell the
broker to prefer to select segments in a tier with a certain
priority.|`highestPriority`|
|`druid.broker.select.tier.custom.priorities`|An array of integer priorities,
such as `[-1, 0, 1, 2]`|Select servers in tiers with a custom priority
list.|The config only has effect if `druid.broker.select.tier` is set to
`custom`. If `druid.broker.select.tier` is set to `custom` but this config is
not specified, the effect is the same as `druid.broker.select.tier` set to
`highestPriority`. Any of the integers in this config can be ignored if there's
no corresponding tiers with such priorities. Tiers with priorities explicitly
specified in this config always have higher priority than those not and those
not specified fall back to use `highestPriority` strategy among themselves.|
+|`druid.broker.select.tier.preferred.tier`| The preferred tier name. E.g.,
`_default_tier` | A non-empty value that specifies the preferred tier in which
historical servers will be picked up for queries. If there are no enough
historical servers from the preferred tier, servers from other tiers (if there
are) will be selected. | null |
Review Comment:
```suggestion
|`druid.broker.select.tier.preferred.tier`| The preferred tier name. E.g.,
`_default_tier` | A non-empty value that specifies the preferred tier in which
historical servers will be picked up for queries. If there are not enough
historical servers from the preferred tier, servers from other tiers (if there
are any) will be selected. This config only has effect if
`druid.broker.select.tier` is set to `preferred` | null |
```
##########
server/src/test/java/org/apache/druid/client/selector/TierSelectorStrategyTest.java:
##########
@@ -260,17 +261,214 @@ public void testServerSelectorStrategyDefaults()
servers.add(p0);
RandomServerSelectorStrategy strategy = new RandomServerSelectorStrategy();
Assert.assertEquals(strategy.pick(servers,
EasyMock.createMock(DataSegment.class)), p0);
- Assert.assertEquals(strategy.pick(EasyMock.createMock(Query.class),
servers, EasyMock.createMock(DataSegment.class)), p0);
- ServerSelectorStrategy defaultDeprecatedServerSelectorStrategy = new
ServerSelectorStrategy() {
+ Assert.assertEquals(
+ strategy.pick(
+ EasyMock.createMock(Query.class),
+ servers,
+ EasyMock.createMock(DataSegment.class)
+ ), p0
+ );
+ ServerSelectorStrategy defaultDeprecatedServerSelectorStrategy = new
ServerSelectorStrategy()
+ {
@Override
- public <T> List<QueryableDruidServer> pick(@Nullable Query<T> query,
Set<QueryableDruidServer> servers, DataSegment segment,
- int numServersToPick)
+ public <T> List<QueryableDruidServer> pick(
+ @Nullable Query<T> query, Collection<QueryableDruidServer> servers,
DataSegment segment,
+ int numServersToPick
+ )
{
return strategy.pick(servers, segment, numServersToPick);
}
};
- Assert.assertEquals(defaultDeprecatedServerSelectorStrategy.pick(servers,
EasyMock.createMock(DataSegment.class)), p0);
- Assert.assertEquals(defaultDeprecatedServerSelectorStrategy.pick(servers,
EasyMock.createMock(DataSegment.class), 1).get(0), p0);
+ Assert.assertEquals(
+ defaultDeprecatedServerSelectorStrategy.pick(servers,
EasyMock.createMock(DataSegment.class)),
+ p0
+ );
+ Assert.assertEquals(
+ defaultDeprecatedServerSelectorStrategy.pick(servers,
EasyMock.createMock(DataSegment.class), 1)
+ .get(0), p0
+ );
+ }
+
+ private void testPreferredTierSelectorStrategy(
Review Comment:
can you add a javadoc specifying a list of what things are being validated
in here?
##########
server/src/main/java/org/apache/druid/client/selector/ServerSelectorStrategy.java:
##########
@@ -38,26 +38,26 @@
public interface ServerSelectorStrategy
{
@Nullable
- default <T> QueryableDruidServer pick(@Nullable Query<T> query,
Set<QueryableDruidServer> servers, DataSegment segment)
+ default <T> QueryableDruidServer pick(@Nullable Query<T> query,
Collection<QueryableDruidServer> servers, DataSegment segment)
Review Comment:
It is not clear to me why we are removing the `Set` restriction here? Your
PR doesn't seem to introduce a change that requires us to now allow duplicate
servers passed to `pick`.. or does it? Maybe I'm missing something.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
