jihoonson commented on a change in pull request #10428:
URL: https://github.com/apache/druid/pull/10428#discussion_r534409548
##########
File path:
server/src/main/java/org/apache/druid/client/selector/TierSelectorStrategy.java
##########
@@ -47,6 +49,22 @@
List<QueryableDruidServer> pick(
Review comment:
Any reason to keep the old interfaces? If this is for
backward-compatibility, it maybe make sense. But please annotate them as
`@Deprecated`.
##########
File path:
server/src/main/java/org/apache/druid/client/selector/TierSelectorStrategy.java
##########
@@ -47,6 +49,22 @@
List<QueryableDruidServer> pick(
Int2ObjectRBTreeMap<Set<QueryableDruidServer>> prioritizedServers,
DataSegment segment,
- int numServersToPick
- );
+ int numServersToPick);
+
+ @Nullable
+ default <T> QueryableDruidServer pick(Query<T> query,
Review comment:
Please annotate query with `@Nullable`.
##########
File path:
server/src/main/java/org/apache/druid/client/selector/TierSelectorStrategy.java
##########
@@ -47,6 +49,22 @@
List<QueryableDruidServer> pick(
Int2ObjectRBTreeMap<Set<QueryableDruidServer>> prioritizedServers,
DataSegment segment,
- int numServersToPick
- );
+ int numServersToPick);
+
+ @Nullable
+ default <T> QueryableDruidServer pick(Query<T> query,
+ Int2ObjectRBTreeMap<Set<QueryableDruidServer>> prioritizedServers,
+ DataSegment segment)
+ {
+ return pick(prioritizedServers, segment);
+ }
+
+ default <T> List<QueryableDruidServer> pick(
+ Query<T> query,
Review comment:
Please add `@Nullable`.
##########
File path:
server/src/main/java/org/apache/druid/client/selector/TierSelectorStrategy.java
##########
@@ -47,6 +49,22 @@
List<QueryableDruidServer> pick(
Int2ObjectRBTreeMap<Set<QueryableDruidServer>> prioritizedServers,
DataSegment segment,
- int numServersToPick
- );
+ int numServersToPick);
+
+ @Nullable
+ default <T> QueryableDruidServer pick(Query<T> query,
+ Int2ObjectRBTreeMap<Set<QueryableDruidServer>> prioritizedServers,
+ DataSegment segment)
+ {
+ return pick(prioritizedServers, segment);
Review comment:
If you want to keep old interfaces, I think the old ones should have a
default implementation that calls this new interface with `null` query. Then we
can remove the implementations of old interfaces in
`AbstractTierSelectorStrategy`.
##########
File path:
server/src/main/java/org/apache/druid/client/selector/ServerSelectorStrategy.java
##########
@@ -33,6 +35,17 @@
})
public interface ServerSelectorStrategy
{
+ default <T> QueryableDruidServer pick(Query<T> query,
Set<QueryableDruidServer> servers, DataSegment segment)
Review comment:
Similarly, I think it would be better that old interfaces have a default
implementation that calls new interfaces with `null` query. Also please
annotate `query` with `@Nullable`.
##########
File path:
server/src/main/java/org/apache/druid/client/selector/ServerSelector.java
##########
@@ -162,12 +163,20 @@ public boolean isEmpty()
@Nullable
@Override
public QueryableDruidServer pick()
+ {
+ if (!historicalServers.isEmpty()) {
+ return strategy.pick(historicalServers, segment.get());
+ }
+ return strategy.pick(realtimeServers, segment.get());
+ }
+
+ public <T> QueryableDruidServer pick(Query<T> query)
Review comment:
It seems that `pick()` is no longer in use. I think we can remove
`pick()` but keep only this new one if `ServerSelector` doesn't implement
`DiscoverySelector`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
