kfaraz commented on code in PR #19654: URL: https://github.com/apache/druid/pull/19654#discussion_r3682635492
########## server/src/main/java/org/apache/druid/server/broker/QueryConfigSnapshot.java: ########## @@ -0,0 +1,72 @@ +/* + * 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.broker; + +import org.apache.druid.query.Query; +import org.apache.druid.query.QueryContexts; +import org.apache.druid.server.QueryBlocklistRule; + +import javax.annotation.Nullable; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Set; + +/** + * A snapshot of the broker's resolved default query context and dynamic config, captured once for a single query so + * that context resolution and the blocklist read one consistent view (rather than re-reading the live config at + * different points in the query lifecycle). The context-resolution logic lives here, keeping it out of + * {@code QueryLifecycle}. On non-broker nodes {@code dynamicConfig} is null (defaults only, no per-query overrides). + */ +public class QueryConfigSnapshot +{ + private final Map<String, Object> defaultContext; + @Nullable + private final BrokerDynamicConfig dynamicConfig; + + public QueryConfigSnapshot(Map<String, Object> defaultContext, @Nullable BrokerDynamicConfig dynamicConfig) + { + this.defaultContext = defaultContext; + this.dynamicConfig = dynamicConfig; + } + + /** + * The final query context. Precedence high to low: a key the client set > per-query dynamic override > the query's Review Comment: Please put the precedence list inside an `<ol>` tag with `<li>` items. ########## server/src/main/java/org/apache/druid/server/broker/BrokerDynamicConfig.java: ########## @@ -91,6 +96,33 @@ public Map<String, PerSegmentTimeoutConfig> getPerSegmentTimeoutConfig() return perSegmentTimeoutConfig; } + /** + * Query-specific broker dynamic config query context overrides (e.g. per segment timeout). + */ + public QueryContext getQuerySpecificContextOverrides(Query<?> query) Review Comment: ```suggestion public QueryContext getContextOverridesForQuery(Query<?> query) ``` ########## server/src/main/java/org/apache/druid/server/broker/BrokerDynamicConfig.java: ########## @@ -91,6 +96,33 @@ public Map<String, PerSegmentTimeoutConfig> getPerSegmentTimeoutConfig() return perSegmentTimeoutConfig; } + /** + * Query-specific broker dynamic config query context overrides (e.g. per segment timeout). + */ + public QueryContext getQuerySpecificContextOverrides(Query<?> query) + { + if (perSegmentTimeoutConfig.isEmpty()) { + return QueryContext.empty(); + } + + for (String tableName : query.getDataSource().getTableNames()) { + PerSegmentTimeoutConfig dsConfig = perSegmentTimeoutConfig.get(tableName); Review Comment: Please use the name `dataSourceTimeoutConfig` instead. ########## server/src/main/java/org/apache/druid/server/broker/BrokerDynamicConfig.java: ########## @@ -91,6 +96,33 @@ public Map<String, PerSegmentTimeoutConfig> getPerSegmentTimeoutConfig() return perSegmentTimeoutConfig; } + /** + * Query-specific broker dynamic config query context overrides (e.g. per segment timeout). + */ + public QueryContext getQuerySpecificContextOverrides(Query<?> query) + { + if (perSegmentTimeoutConfig.isEmpty()) { + return QueryContext.empty(); + } + + for (String tableName : query.getDataSource().getTableNames()) { + PerSegmentTimeoutConfig dsConfig = perSegmentTimeoutConfig.get(tableName); + if (dsConfig != null) { + if (dsConfig.isMonitorOnly()) { + log.debug( + "Per-segment timeout [%d ms] configured for datasource [%s] in monitorOnly mode (not enforced) for query [%s].", Review Comment: ```suggestion "Per-segment timeout [%d ms] configured for datasource[%s] in monitorOnly mode (not enforced) for query[%s].", ``` ########## server/src/main/java/org/apache/druid/server/QueryLifecycle.java: ########## @@ -212,61 +220,41 @@ public void after(final boolean isDone, final Throwable thrown) * @throws DruidException if the current state is not NEW, which indicates a bug */ public void initialize(final Query<?> baseQuery) + { + initialize(baseQuery, null); + } + + /** + * As {@link #initialize(Query)}, but takes the context keys the client actually set. Pass {@code null} to treat the + * whole context as client-set (native queries). The SQL layer merges static defaults into the context, so it must + * pass the real client-set keys so dynamic overrides can beat a merged-in default without overriding the client. + * + * @throws DruidException if the current state is not NEW, which indicates a bug + */ + public void initialize(final Query<?> baseQuery, @Nullable final Set<String> clientProvidedQueryContextKeys) { transition(State.NEW, State.INITIALIZED); - userContextKeys = new HashSet<>(baseQuery.getContext().keySet()); + final Map<String, Object> baseContext = baseQuery.getContext(); + authorizationContextKeys = new HashSet<>(baseContext.keySet()); + + // Keys the client actually set (native queries pass null, so the whole context is client-set). + final Set<String> effectiveClientProvidedQueryContextKeys = + clientProvidedQueryContextKeys != null ? clientProvidedQueryContextKeys : baseContext.keySet(); + String queryId = baseQuery.getId(); if (Strings.isNullOrEmpty(queryId)) { queryId = UUID.randomUUID().toString(); } - // Start with system defaults, apply per-datasource override, then user context wins - Map<String, Object> contextWithDefaults = new HashMap<>(queryConfigProvider.getContext()); - applyPerDatasourcePerSegmentTimeout(baseQuery, contextWithDefaults, queryId); - Map<String, Object> finalContext = QueryContexts.override(contextWithDefaults, baseQuery.getContext()); Review Comment: @jtuglu1 , I assume here was the bug that we are trying to fix. IIUC, since we didn't have the exact set of `clientProvidedQueryContextKeys`, we assumed that everything in the `baseQuery` was set by the client and the per-datasource-per-segment timeout in the dynamic config would never get applied. Is that understanding correct? ########## server/src/main/java/org/apache/druid/client/BrokerViewOfBrokerConfig.java: ########## Review Comment: Does this method still need to be `synchronized`? ########## server/src/main/java/org/apache/druid/server/QueryLifecycle.java: ########## @@ -113,8 +108,11 @@ public class QueryLifecycle @MonotonicNonNull private Query<?> baseQuery; + /** + * Keys present on the query context as received; the candidate set fed to context-key authorization. + */ @MonotonicNonNull - private Set<String> userContextKeys; + private Set<String> authorizationContextKeys; Review Comment: Should this be called `authorizedUserContextKeys` or similar? ########## server/src/main/java/org/apache/druid/client/BrokerViewOfBrokerConfig.java: ########## @@ -127,4 +128,13 @@ public Map<String, Object> getContext() { return resolvedDefaultQueryContext.asMap(); } + + /** + * Captures the resolved default context and the dynamic config atomically, so a single query resolves its context + * and blocklist against one consistent snapshot rather than re-reading the live config. + */ + public synchronized QueryConfigSnapshot snapshotForQuery() Review Comment: Probably need not be synchronized given the recent changes in #19787 . ########## server/src/main/java/org/apache/druid/server/broker/QueryConfigSnapshot.java: ########## @@ -0,0 +1,72 @@ +/* + * 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.broker; + +import org.apache.druid.query.Query; +import org.apache.druid.query.QueryContexts; +import org.apache.druid.server.QueryBlocklistRule; + +import javax.annotation.Nullable; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Set; + +/** + * A snapshot of the broker's resolved default query context and dynamic config, captured once for a single query so + * that context resolution and the blocklist read one consistent view (rather than re-reading the live config at + * different points in the query lifecycle). The context-resolution logic lives here, keeping it out of + * {@code QueryLifecycle}. On non-broker nodes {@code dynamicConfig} is null (defaults only, no per-query overrides). Review Comment: Nit: rephrase for simplicity: ```suggestion * A snapshot of the {@link BrokerDynamicConfig} (null on non-Broker nodes) * and the resolved default query context that is used for the entire {@code QueryLifecycle} * of a single query. ``` ########## server/src/main/java/org/apache/druid/server/broker/BrokerDynamicConfig.java: ########## @@ -91,6 +96,33 @@ public Map<String, PerSegmentTimeoutConfig> getPerSegmentTimeoutConfig() return perSegmentTimeoutConfig; } + /** + * Query-specific broker dynamic config query context overrides (e.g. per segment timeout). + */ + public QueryContext getQuerySpecificContextOverrides(Query<?> query) + { + if (perSegmentTimeoutConfig.isEmpty()) { + return QueryContext.empty(); + } + + for (String tableName : query.getDataSource().getTableNames()) { + PerSegmentTimeoutConfig dsConfig = perSegmentTimeoutConfig.get(tableName); + if (dsConfig != null) { + if (dsConfig.isMonitorOnly()) { + log.debug( Review Comment: Does this really warrant a log, even debug? ########## server/src/main/java/org/apache/druid/server/broker/QueryConfigSnapshot.java: ########## @@ -0,0 +1,72 @@ +/* + * 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.broker; + +import org.apache.druid.query.Query; +import org.apache.druid.query.QueryContexts; +import org.apache.druid.server.QueryBlocklistRule; + +import javax.annotation.Nullable; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Set; + +/** + * A snapshot of the broker's resolved default query context and dynamic config, captured once for a single query so + * that context resolution and the blocklist read one consistent view (rather than re-reading the live config at + * different points in the query lifecycle). The context-resolution logic lives here, keeping it out of + * {@code QueryLifecycle}. On non-broker nodes {@code dynamicConfig} is null (defaults only, no per-query overrides). + */ +public class QueryConfigSnapshot +{ + private final Map<String, Object> defaultContext; Review Comment: Please rename this to `resolvedDefaultQueryContext` to avoid confusion and indicate that it has already been resolved with `BrokerDynamicConfig.getContext()`. ########## server/src/main/java/org/apache/druid/server/broker/QueryConfigSnapshot.java: ########## @@ -0,0 +1,72 @@ +/* + * 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.broker; + +import org.apache.druid.query.Query; +import org.apache.druid.query.QueryContexts; +import org.apache.druid.server.QueryBlocklistRule; + +import javax.annotation.Nullable; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Set; + +/** + * A snapshot of the broker's resolved default query context and dynamic config, captured once for a single query so + * that context resolution and the blocklist read one consistent view (rather than re-reading the live config at + * different points in the query lifecycle). The context-resolution logic lives here, keeping it out of + * {@code QueryLifecycle}. On non-broker nodes {@code dynamicConfig} is null (defaults only, no per-query overrides). + */ +public class QueryConfigSnapshot +{ + private final Map<String, Object> defaultContext; + @Nullable + private final BrokerDynamicConfig dynamicConfig; + + public QueryConfigSnapshot(Map<String, Object> defaultContext, @Nullable BrokerDynamicConfig dynamicConfig) + { + this.defaultContext = defaultContext; + this.dynamicConfig = dynamicConfig; + } + + /** + * The final query context. Precedence high to low: a key the client set > per-query dynamic override > the query's Review Comment: I suppose my confusion might be stemming from the fact that the `BrokerDynamicConfig` has a dedicated `context` field but also provides query-specific overrides. ########## server/src/main/java/org/apache/druid/server/broker/QueryConfigSnapshot.java: ########## @@ -0,0 +1,72 @@ +/* + * 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.broker; + +import org.apache.druid.query.Query; +import org.apache.druid.query.QueryContexts; +import org.apache.druid.server.QueryBlocklistRule; + +import javax.annotation.Nullable; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Set; + +/** + * A snapshot of the broker's resolved default query context and dynamic config, captured once for a single query so + * that context resolution and the blocklist read one consistent view (rather than re-reading the live config at + * different points in the query lifecycle). The context-resolution logic lives here, keeping it out of + * {@code QueryLifecycle}. On non-broker nodes {@code dynamicConfig} is null (defaults only, no per-query overrides). + */ +public class QueryConfigSnapshot +{ + private final Map<String, Object> defaultContext; + @Nullable + private final BrokerDynamicConfig dynamicConfig; + + public QueryConfigSnapshot(Map<String, Object> defaultContext, @Nullable BrokerDynamicConfig dynamicConfig) + { + this.defaultContext = defaultContext; + this.dynamicConfig = dynamicConfig; + } + + /** + * The final query context. Precedence high to low: a key the client set > per-query dynamic override > the query's Review Comment: Why is the query's own context the 3rd item in this list? As the PR description mentions, I think the context precedence order should be: query payload > dynamic config > runtime property > code defaults. -- 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]
