kfaraz commented on code in PR #17899:
URL: https://github.com/apache/druid/pull/17899#discussion_r2037154705
##########
server/src/main/java/org/apache/druid/client/coordinator/CoordinatorClient.java:
##########
@@ -87,4 +88,8 @@ public interface CoordinatorClient
*/
ListenableFuture<CompactionStatusResponse> getCompactionSnapshots(@Nullable
String dataSource);
+ /**
+ * TODO: javadoc
+ */
+ ListenableFuture<CoordinatorDynamicConfig> getCoordinatorConfig();
Review Comment:
```suggestion
ListenableFuture<CoordinatorDynamicConfig> getCoordinatorDynamicConfig();
```
##########
server/src/main/java/org/apache/druid/client/DynamicConfigurationManager.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.client;
+
+import com.google.inject.Inject;
+import org.apache.druid.client.coordinator.CoordinatorClient;
+import org.apache.druid.java.util.common.lifecycle.LifecycleStart;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.server.coordinator.CoordinatorDynamicConfig;
+
+import java.util.HashSet;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
+
+public class DynamicConfigurationManager
Review Comment:
```suggestion
public class CoordinatorDynamicConfigView
```
##########
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:
+1
##########
server/src/main/java/org/apache/druid/client/DynamicConfigurationManager.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.client;
+
+import com.google.inject.Inject;
+import org.apache.druid.client.coordinator.CoordinatorClient;
+import org.apache.druid.java.util.common.lifecycle.LifecycleStart;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.server.coordinator.CoordinatorDynamicConfig;
+
+import java.util.HashSet;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
+
+public class DynamicConfigurationManager
Review Comment:
Based on the other comment, this class should perhaps implement the
`HistoricalFilter` interface.
##########
server/src/test/java/org/apache/druid/server/metrics/TestDynamicConfigurationManager.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.server.metrics;
+
+import com.google.common.collect.ImmutableSet;
+import org.apache.druid.client.DynamicConfigurationManager;
+import org.apache.druid.server.coordinator.CoordinatorDynamicConfig;
+
+import java.util.Set;
+
+public class TestDynamicConfigurationManager extends
DynamicConfigurationManager
Review Comment:
I think the concrete implementation `DynamicConfigurationManager` is simple
enough to not require a dedicated test implementation.
##########
server/src/main/java/org/apache/druid/server/DruidInternalDynamicConfigResource.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.server;
+
+import com.google.inject.Inject;
+import org.apache.druid.client.DynamicConfigurationManager;
+import org.apache.druid.server.coordinator.CoordinatorDynamicConfig;
+
+import javax.ws.rs.Consumes;
+import javax.ws.rs.GET;
+import javax.ws.rs.POST;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.Response;
+
+@Path("/druid-internal/v1/dynamicConfiguration")
+public class DruidInternalDynamicConfigResource
Review Comment:
I don't think we need a new resource class for this. We can just add the new
methods to `BrokerResource`.
##########
processing/src/main/java/org/apache/druid/query/QueryContexts.java:
##########
@@ -149,6 +150,7 @@ public class QueryContexts
public static final boolean DEFAULT_ENABLE_JOIN_FILTER_PUSH_DOWN = true;
public static final boolean DEFAULT_ENABLE_JOIN_FILTER_REWRITE = true;
public static final boolean
DEFAULT_ENABLE_JOIN_FILTER_REWRITE_VALUE_COLUMN_FILTERS = false;
+ public static final boolean DEFAULT_QUERY_UNMANAGED_SERVERS = false;
Review Comment:
Rather than a boolean, I think we should use an enum `QueryCloneMode` with
values:
```
ONLY: Query ONLY the clones
INCLUDE: Query the clones and normal servers
EXCLUDE (default): Do not query the clones
```
##########
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:
Yeah, I suppose we can hook into the `AbstractTierSelectorStrategy` too if
we want to avoid creating the maps every time.
Although, this map will always be small (1 or 2 entries) for any given
segment, because it is dictated by the replication factor. So, I am not sure if
the impact would really be perceptible. But I still wouldn't mind the
improvement.
The only con of mixing the `HistoricalFilter` with the
`TierSelectorStrategy` is that we lose the separation of concern and it
pollutes the logic a little. Each `TierSelectorStrategy` implementation will
need to have a `@JacksonInject HistoricalFilter` arg and pass it up to the
super class to use in the `pick` implementation.
##########
server/src/main/java/org/apache/druid/client/selector/HistoricalFilter.java:
##########
@@ -0,0 +1,39 @@
+package org.apache.druid.client.selector;
+
+import com.google.common.collect.ImmutableSet;
+import it.unimi.dsi.fastutil.ints.Int2ObjectRBTreeMap;
+import org.apache.druid.client.QueryableDruidServer;
+
+import java.util.Set;
+import java.util.function.Function;
+import java.util.function.Supplier;
+import java.util.stream.Collectors;
+
+public class HistoricalFilter implements
Function<Int2ObjectRBTreeMap<Set<QueryableDruidServer>>,
Int2ObjectRBTreeMap<Set<QueryableDruidServer>>>
Review Comment:
Make `HistoricalFilter` an interface with a method `getQueryableServers`.
The implementation would be the `DynamicConfigManager` class itself.
```java
public Set<QueryableDruidServer>
getQueryableServers(Set<QueryableDruidServer> allServers, CloneQueryMode mode) {
// read the coordinator dynamic config here and ignore the servers from
allServers as applicable
}
```
##########
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:
I think atomic reference should be faster:
https://stackoverflow.com/a/31578070
That said, for all the cases, the numbers are small enough (in 2-digit
nanos) to not really matter.
I think the question becomes more relevant if we are doing some operations
inside the lock.
--
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]
