You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/09/23 21:17:48 UTC
[GitHub] [druid] kroeders opened a new pull request #10428: allow server selection to be aware of query
kroeders opened a new pull request #10428:
URL: https://github.com/apache/druid/pull/10428
### Description
This PR supports https://github.com/apache/druid/issues/10294 where an issue is discussed that queries with lookups may sometimes fail unnecessarily due to the lookup only being available on some historicals. The query can succeed by avoiding those historicals. In this PR is an extension that tracks lookup status on the broker and adds a new ServerSelectorStrategy to avoid historicals that are missing required lookups.
This PR is a small change to make server selection aware of the Query and would allow this extension so solve this issue without any other changes in core Druid.
This change just adds Query as a parameter to the ServerSelectorStratey and TierSelectorStrategy and provides this parameter in CachingClusteredClient, after segments are found for a query and they are being fanned out across the cluster. A default method was added to the interface so existing implementations would not need to change.
<hr>
This PR has:
- [X] been self-reviewed.
- [X] 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.
- [ ] been tested in a test Druid cluster.
<hr>
##### Key changed/added classes in this PR
* `ServerSelectorStrategy`
* `TierSelectorStrategy`
* `CachingClusteredClient`
----------------------------------------------------------------
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:
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] kroeders commented on pull request #10428: allow server selection to be aware of query
Posted by GitBox <gi...@apache.org>.
kroeders commented on pull request #10428:
URL: https://github.com/apache/druid/pull/10428#issuecomment-729041005
thanks for taking a look @nishantmonu51 !
----------------------------------------------------------------
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:
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] kroeders commented on pull request #10428: allow server selection to be aware of query
Posted by GitBox <gi...@apache.org>.
kroeders commented on pull request #10428:
URL: https://github.com/apache/druid/pull/10428#issuecomment-739550336
fixing a few test issues with the default methods...
----------------------------------------------------------------
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:
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] kroeders commented on a change in pull request #10428: allow server selection to be aware of query
Posted by GitBox <gi...@apache.org>.
kroeders commented on a change in pull request #10428:
URL: https://github.com/apache/druid/pull/10428#discussion_r535246123
##########
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:
I removed it, the method was called a couple of times in tests (which seems OK) and in the etag creation part, which seems OK to me. It looks a little funny to pass in null as a single parameter, but I could argue either way vs having a method that isn't used elsewhere.
##########
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:
done
----------------------------------------------------------------
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:
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] jihoonson commented on a change in pull request #10428: allow server selection to be aware of query
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10428:
URL: https://github.com/apache/druid/pull/10428#discussion_r545495727
##########
File path: server/src/main/java/org/apache/druid/client/CachingClusteredClient.java
##########
@@ -812,7 +812,8 @@ String computeResultLevelCachingEtag(
Hasher hasher = Hashing.sha1().newHasher();
boolean hasOnlyHistoricalSegments = true;
for (SegmentServerSelector p : segments) {
- if (!p.getServer().pick().getServer().isSegmentReplicationTarget()) {
+ QueryableDruidServer queryableServer = p.getServer().pick(query);
+ if (queryableServer == null || !queryableServer.getServer().isSegmentReplicationTarget()) {
Review comment:
nit: I think it should ideally fail if `queryableServer` is null, because it means there is no server to process this segment. However, this seems OK because this method will return immediately when it's null and fail at [here](https://github.com/apache/druid/pull/10428/files#diff-d0873a0b1d76e92318ed707d876a168cf8ad974a4c53ebe65d7a39ab3cf0294eR594).
----------------------------------------------------------------
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:
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] jihoonson commented on a change in pull request #10428: allow server selection to be aware of query
Posted by GitBox <gi...@apache.org>.
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:
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] kroeders commented on a change in pull request #10428: allow server selection to be aware of query
Posted by GitBox <gi...@apache.org>.
kroeders commented on a change in pull request #10428:
URL: https://github.com/apache/druid/pull/10428#discussion_r537064006
##########
File path: server/src/main/java/org/apache/druid/client/selector/ServerSelector.java
##########
@@ -159,15 +160,13 @@ public boolean isEmpty()
return servers;
}
- @Nullable
- @Override
- public QueryableDruidServer pick()
+ public <T> QueryableDruidServer pick(@Nullable Query<T> query)
Review comment:
yep, it sure can! thanks
----------------------------------------------------------------
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:
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] kroeders commented on a change in pull request #10428: allow server selection to be aware of query
Posted by GitBox <gi...@apache.org>.
kroeders commented on a change in pull request #10428:
URL: https://github.com/apache/druid/pull/10428#discussion_r535244203
##########
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:
done
----------------------------------------------------------------
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:
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] kroeders commented on pull request #10428: allow server selection to be aware of query
Posted by GitBox <gi...@apache.org>.
kroeders commented on pull request #10428:
URL: https://github.com/apache/druid/pull/10428#issuecomment-748151440
@jihoonson no problem, thanks again for working with me on this one!
----------------------------------------------------------------
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:
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] kroeders commented on a change in pull request #10428: allow server selection to be aware of query
Posted by GitBox <gi...@apache.org>.
kroeders commented on a change in pull request #10428:
URL: https://github.com/apache/druid/pull/10428#discussion_r537071548
##########
File path: server/src/main/java/org/apache/druid/client/CachingClusteredClient.java
##########
@@ -812,7 +812,7 @@ String computeResultLevelCachingEtag(
Hasher hasher = Hashing.sha1().newHasher();
boolean hasOnlyHistoricalSegments = true;
for (SegmentServerSelector p : segments) {
- if (!p.getServer().pick().getServer().isSegmentReplicationTarget()) {
+ if (!p.getServer().pick(null).getServer().isSegmentReplicationTarget()) {
Review comment:
I made the query change.
For the null check I was thinking for this part p.getServer().pick(query) since we just annotated this Nullable below?
Something like -
QueryableServer queryableServer = p.getServer().pick(query);
if (queryableServer==null || !queryableServer.getServer().isSegmentReplicationTarget()) {
----------------------------------------------------------------
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:
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] kroeders commented on pull request #10428: allow server selection to be aware of query
Posted by GitBox <gi...@apache.org>.
kroeders commented on pull request #10428:
URL: https://github.com/apache/druid/pull/10428#issuecomment-740742545
hi @jihoonson I think I've got them all here. I also added@Nullable to the ServerSelectorStrategy method that leads to this other Nullable and added a couple tests. Please let me know if you see anything else and thanks again for working with me on this one
----------------------------------------------------------------
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:
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] kroeders commented on a change in pull request #10428: allow server selection to be aware of query
Posted by GitBox <gi...@apache.org>.
kroeders commented on a change in pull request #10428:
URL: https://github.com/apache/druid/pull/10428#discussion_r537064303
##########
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:
works for me, current one it is!
----------------------------------------------------------------
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:
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] kroeders commented on a change in pull request #10428: allow server selection to be aware of query
Posted by GitBox <gi...@apache.org>.
kroeders commented on a change in pull request #10428:
URL: https://github.com/apache/druid/pull/10428#discussion_r535252310
##########
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:
This was done for backward compatibility - I was trying to make this change as minimal as possible on the first cut. We don't have any custom implementations here and I haven't seen any on a cursory look, but I don't want to presume. I think it's cleaner to remove the ones that don't take Query and having an interface of all defaults that call each other makes the abstract methods less obvious. What do you think? I'm happy to defer to your judgement.
----------------------------------------------------------------
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:
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] kroeders commented on a change in pull request #10428: allow server selection to be aware of query
Posted by GitBox <gi...@apache.org>.
kroeders commented on a change in pull request #10428:
URL: https://github.com/apache/druid/pull/10428#discussion_r535243969
##########
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:
done
----------------------------------------------------------------
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:
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] jihoonson commented on pull request #10428: allow server selection to be aware of query
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10428:
URL: https://github.com/apache/druid/pull/10428#issuecomment-748338837
@kroeders I just merged. Thank you!
----------------------------------------------------------------
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:
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] jihoonson merged pull request #10428: allow server selection to be aware of query
Posted by GitBox <gi...@apache.org>.
jihoonson merged pull request #10428:
URL: https://github.com/apache/druid/pull/10428
----------------------------------------------------------------
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:
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] kroeders commented on pull request #10428: allow server selection to be aware of query
Posted by GitBox <gi...@apache.org>.
kroeders commented on pull request #10428:
URL: https://github.com/apache/druid/pull/10428#issuecomment-738015529
thanks for the feedback and taking the time to look @jihoonson , I appreciate it! I updated the branch per the comments and left a few notes that I found while making the changes.
----------------------------------------------------------------
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:
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] kroeders commented on pull request #10428: allow server selection to be aware of query
Posted by GitBox <gi...@apache.org>.
kroeders commented on pull request #10428:
URL: https://github.com/apache/druid/pull/10428#issuecomment-747765658
hi @jihoonson no worries of course, thanks for working with me on this! This will let us package the lookup change as an extension and deploy it internally. I added the null check and pushed here, happy holidays
----------------------------------------------------------------
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:
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] jihoonson commented on a change in pull request #10428: allow server selection to be aware of query
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10428:
URL: https://github.com/apache/druid/pull/10428#discussion_r545452210
##########
File path: server/src/main/java/org/apache/druid/client/CachingClusteredClient.java
##########
@@ -812,7 +812,7 @@ String computeResultLevelCachingEtag(
Hasher hasher = Hashing.sha1().newHasher();
boolean hasOnlyHistoricalSegments = true;
for (SegmentServerSelector p : segments) {
- if (!p.getServer().pick().getServer().isSegmentReplicationTarget()) {
+ if (!p.getServer().pick(null).getServer().isSegmentReplicationTarget()) {
Review comment:
That's a good catch! I think we should add a null check.
----------------------------------------------------------------
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:
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] kroeders commented on pull request #10428: allow server selection to be aware of query
Posted by GitBox <gi...@apache.org>.
kroeders commented on pull request #10428:
URL: https://github.com/apache/druid/pull/10428#issuecomment-748372031
awesome, thanks!
----------------------------------------------------------------
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:
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] jihoonson commented on a change in pull request #10428: allow server selection to be aware of query
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10428:
URL: https://github.com/apache/druid/pull/10428#discussion_r536489478
##########
File path: server/src/main/java/org/apache/druid/client/selector/ServerSelector.java
##########
@@ -159,15 +160,13 @@ public boolean isEmpty()
return servers;
}
- @Nullable
- @Override
- public QueryableDruidServer pick()
+ public <T> QueryableDruidServer pick(@Nullable Query<T> query)
Review comment:
It seems that this can still return null?
##########
File path: server/src/main/java/org/apache/druid/client/CachingClusteredClient.java
##########
@@ -812,7 +812,7 @@ String computeResultLevelCachingEtag(
Hasher hasher = Hashing.sha1().newHasher();
boolean hasOnlyHistoricalSegments = true;
for (SegmentServerSelector p : segments) {
- if (!p.getServer().pick().getServer().isSegmentReplicationTarget()) {
+ if (!p.getServer().pick(null).getServer().isSegmentReplicationTarget()) {
Review comment:
It sounds good to me. For the null check, do you mean a null check on the query? If so, it doesn't seem ncessary because query cannot be null in `CachingClusteredClient`.
##########
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:
I don't have strong opinion here. The current one looks good to me.
----------------------------------------------------------------
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:
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] kroeders commented on a change in pull request #10428: allow server selection to be aware of query
Posted by GitBox <gi...@apache.org>.
kroeders commented on a change in pull request #10428:
URL: https://github.com/apache/druid/pull/10428#discussion_r535246679
##########
File path: server/src/main/java/org/apache/druid/client/selector/TierSelectorStrategy.java
##########
@@ -47,6 +49,22 @@
List<QueryableDruidServer> pick(
Review comment:
I marked the non-query ones as deprecated throughout
----------------------------------------------------------------
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:
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] kroeders commented on a change in pull request #10428: allow server selection to be aware of query
Posted by GitBox <gi...@apache.org>.
kroeders commented on a change in pull request #10428:
URL: https://github.com/apache/druid/pull/10428#discussion_r535256895
##########
File path: server/src/main/java/org/apache/druid/client/CachingClusteredClient.java
##########
@@ -812,7 +812,7 @@ String computeResultLevelCachingEtag(
Hasher hasher = Hashing.sha1().newHasher();
boolean hasOnlyHistoricalSegments = true;
for (SegmentServerSelector p : segments) {
- if (!p.getServer().pick().getServer().isSegmentReplicationTarget()) {
+ if (!p.getServer().pick(null).getServer().isSegmentReplicationTarget()) {
Review comment:
this was the only functional place that the parameterless pick() was called that I mentioned in the other comment @jihoonson , the Query is available inside of SpecificQueryRunnable where this is called, so I could alternatively pass the query into computeResultLevelCachingEtag and add a null check on this line, what do you think?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org