cryptoe commented on code in PR #17899:
URL: https://github.com/apache/druid/pull/17899#discussion_r2037383844
##########
server/src/main/java/org/apache/druid/client/CachingClusteredClient.java:
##########
@@ -598,7 +605,18 @@ private SortedMap<DruidServer, List<SegmentDescriptor>>
groupSegmentsByServer(Se
{
final SortedMap<DruidServer, List<SegmentDescriptor>> serverSegments =
new TreeMap<>();
for (SegmentServerSelector segmentServer : segments) {
- final QueryableDruidServer queryableDruidServer =
segmentServer.getServer().pick(query);
+ Supplier<Set<String>> supplier = () -> {
+ boolean queryUnmanagedServers =
query.context().getQueryUnmanagedServers();
+ if (queryUnmanagedServers) {
+ // If this is a test query, ignore source servers.
Review Comment:
I think it need not be a test query.
```suggestion
```
##########
server/src/main/java/org/apache/druid/client/CachingClusteredClient.java:
##########
@@ -598,7 +605,18 @@ private SortedMap<DruidServer, List<SegmentDescriptor>>
groupSegmentsByServer(Se
{
final SortedMap<DruidServer, List<SegmentDescriptor>> serverSegments =
new TreeMap<>();
for (SegmentServerSelector segmentServer : segments) {
- final QueryableDruidServer queryableDruidServer =
segmentServer.getServer().pick(query);
+ Supplier<Set<String>> supplier = () -> {
+ boolean queryUnmanagedServers =
query.context().getQueryUnmanagedServers();
+ if (queryUnmanagedServers) {
+ // If this is a test query, ignore source servers.
+ return dynamicConfigurationManager.getSourceClusterServers();
+ } else {
+ return dynamicConfigurationManager.getTargetCloneServers();
Review Comment:
Currently this is getting the list of historicals from na atom reference.
Since this is the hot code, can we change the dynamic configuration manager to
a read write lock and see the impact. I see you have a benchmark here as well.
What are the results ?
##########
server/src/main/java/org/apache/druid/client/selector/ServerSelector.java:
##########
@@ -122,12 +122,17 @@
}
public List<DruidServerMetadata> getCandidates(final int numCandidates)
+ {
+ return getCandidates(numCandidates, HistoricalFilter.IDENTITIY_FILTER);
+ }
+
+ public List<DruidServerMetadata> getCandidates(final int numCandidates,
HistoricalFilter filter)
{
List<DruidServerMetadata> candidates;
synchronized (this) {
if (numCandidates > 0) {
candidates = new ArrayList<>(numCandidates);
- strategy.pick(historicalServers, segment.get(), numCandidates)
+ strategy.pick(filter.apply(historicalServers), segment.get(),
numCandidates)
Review Comment:
Currently I am not sure which is cheaper. Making changes here or :
https://github.com/implydata/druid/blob/ef72cff603cac88c0eca6da7ed1f4a5f814d87bc/server/src/main/java/org/apache/druid/client/selector/AbstractTierSelectorStrategy.java#L65.
I guess creating a new Int2ObjectRBTreeMap might be more expensive.
--
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]
