gianm commented on code in PR #19144:
URL: https://github.com/apache/druid/pull/19144#discussion_r2929626959
##########
server/src/main/java/org/apache/druid/server/QueryLifecycleFactory.java:
##########
@@ -94,7 +98,7 @@ public QueryLifecycle factorize()
emitter,
requestLogger,
authorizerMapper,
- defaultQueryConfig,
+ effectiveDefaultQueryConfig,
Review Comment:
`DefaultQueryConfig#getContext` is used in many places beyond just here.
Please catch them all somehow.
One way: how about replacing all current usages of `DefaultQueryConfig` with
a new interface `DefaultQueryContext`? Then make `BrokerViewOfBrokerConfig`
implement that interface, and bind it to the interface in Guice. Then all
usages will get the correct merged default context.
##########
docs/api-reference/dynamic-configuration-api.md:
##########
@@ -306,7 +306,7 @@ Host: http://ROUTER_IP:ROUTER_PORT
## Broker dynamic configuration
Broker dynamic configuration is managed through the Coordinator but consumed
by Brokers.
-These settings control broker behavior such as query blocking rules.
+These settings control broker behavior such as query blocking rules and
default query context values.
Review Comment:
For the blocking we have this warning:
> **Note:** Query blocking is best-effort. Queries may not be blocked in
certain cases, such as when a Broker has recently started and hasn't received
the config yet, or if the Broker cannot contact the Coordinator. Brokers poll
the configuration periodically (default every 1 minute) and also receive push
updates from the Coordinator for immediate propagation.
I think we need a similar warning for context. Maybe put this warning at the
top of the "Broker dynamic configuration" section and make clear it applies to
everything in here. I say this because in case default query context properties
are "important" (i.e. operators really want them to always be set) they may be
surprised that they are in fact not necessarily always going to be set.
This also makes me think at some point we might want to add a mode to make
them actually guaranteed, i.e., block Broker startup until first sync of
dynamic config. Doesn't have to be done in this PR.
##########
server/src/main/java/org/apache/druid/client/BrokerViewOfBrokerConfig.java:
##########
@@ -21,34 +21,50 @@
import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.common.annotations.VisibleForTesting;
+import com.google.errorprone.annotations.concurrent.GuardedBy;
import com.google.inject.Inject;
import org.apache.druid.client.coordinator.Coordinator;
import org.apache.druid.client.coordinator.CoordinatorClient;
import org.apache.druid.client.coordinator.CoordinatorClientImpl;
import org.apache.druid.discovery.NodeRole;
import org.apache.druid.guice.annotations.EscalatedGlobal;
import org.apache.druid.guice.annotations.Json;
+import org.apache.druid.query.DefaultQueryConfig;
+import org.apache.druid.query.QueryContexts;
import org.apache.druid.rpc.ServiceClientFactory;
import org.apache.druid.rpc.ServiceLocator;
import org.apache.druid.rpc.StandardRetryPolicy;
import org.apache.druid.server.broker.BrokerDynamicConfig;
import javax.validation.constraints.NotNull;
+import java.util.Map;
/**
* Broker view of broker dynamic configuration.
*/
public class BrokerViewOfBrokerConfig extends
BaseBrokerViewOfConfig<BrokerDynamicConfig>
{
private final CoordinatorClient coordinatorClient;
+ private final DefaultQueryConfig defaultQueryConfig;
+
+ /**
+ * Pre-computed merge of {@link DefaultQueryConfig#getContext()} and
+ * {@link BrokerDynamicConfig#getQueryContext()}, recomputed on each config
sync.
+ * Dynamic config values override static defaults.
+ */
+ @GuardedBy("this")
+ private Map<String, Object> resolvedDefaultQueryContext;
Review Comment:
Given how this is used, it would be better to have it be a `volatile`, no
`@GuardedBy`, and be an `ImmutableMap`. That way there will not be any
potential for thread contention when reading it.
--
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]
