You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "LakshSingla (via GitHub)" <gi...@apache.org> on 2023/08/14 05:41:02 UTC
[PR] Auto configuration for maxSubqueryBytes (druid)
LakshSingla opened a new pull request, #14808:
URL: https://github.com/apache/druid/pull/14808
### Description
This PR adds the ability to configure the `maxSubqueryBytes` parameter with a the following configurations:
1. `unlimited`: Same as unbounded, and defers to the original behavior.
2. `auto` : Instead of manually setting the maxSubqueryBytes, the user can set it to apportion a fixed memory of the heap (which is calculated automatically) for materializing the results of the subqueries for incoming queries. This should cover most of the cases and allow the user to experiment with the new feature with minimum friction.
3. Some specific numerical value: (Was present originally) - Reserve the provided portion of bytes per query for materializing the subquery results. This can be used for fine-tuning if the query patterns are known. For example, if the operator knows that the cluster has only a couple of concurrent subqueries, then they can configure it to a larger value than set by `auto`.
#### Release note
To be added
<hr>
##### Key changed/added classes in this PR
* `SubqueryLimitUtils`
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] 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.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Auto configuration for maxSubqueryBytes (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14808:
URL: https://github.com/apache/druid/pull/14808#discussion_r1306937738
##########
server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java:
##########
@@ -127,6 +137,12 @@ public ClientQuerySegmentWalker(
this.serverConfig = serverConfig;
this.cache = cache;
this.cacheConfig = cacheConfig;
+ this.subqueryLimitUtils = new SubqueryLimitUtils(
Review Comment:
I made the `SubqueryLimitUtils` class non-static since we compute the size of the lookup at the start, and therefore it's required to have an internal state. Perhaps renaming it to `SubqueryLimitHelper` would be more appropriate.
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Auto configuration for maxSubqueryBytes (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #14808:
URL: https://github.com/apache/druid/pull/14808#discussion_r1330317502
##########
server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java:
##########
@@ -127,6 +137,12 @@ public ClientQuerySegmentWalker(
this.serverConfig = serverConfig;
this.cache = cache;
this.cacheConfig = cacheConfig;
+ this.subqueryGuardrailHelper = new SubqueryGuardrailHelper(
+ lookupManager,
+ Runtime.getRuntime().maxMemory(),
+ httpClientConfig.getNumConnections()
Review Comment:
This is the # of connections from the Broker to Historicals. I think we should be using the # of queries, though? That's `ServerConfig#getNumThreads`, a.k.a. `druid.server.http.numThreads`.
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Auto configuration for maxSubqueryBytes (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #14808:
URL: https://github.com/apache/druid/pull/14808#discussion_r1330317502
##########
server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java:
##########
@@ -127,6 +137,12 @@ public ClientQuerySegmentWalker(
this.serverConfig = serverConfig;
this.cache = cache;
this.cacheConfig = cacheConfig;
+ this.subqueryGuardrailHelper = new SubqueryGuardrailHelper(
+ lookupManager,
+ Runtime.getRuntime().maxMemory(),
+ httpClientConfig.getNumConnections()
Review Comment:
This is the # of connections from the Broker to each Historical. I think we should be using the # of possible concurrent queries, though? That's `ServerConfig#getNumThreads`, a.k.a. `druid.server.http.numThreads`.
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Auto configuration for maxSubqueryBytes (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #14808:
URL: https://github.com/apache/druid/pull/14808#discussion_r1314527054
##########
server/src/main/java/org/apache/druid/server/SubqueryLimitUtils.java:
##########
@@ -0,0 +1,132 @@
+/*
+ * 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 org.apache.druid.error.InvalidInput;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.query.lookup.LookupExtractor;
+import org.apache.druid.query.lookup.LookupExtractorFactoryContainer;
+import org.apache.druid.query.lookup.LookupExtractorFactoryContainerProvider;
+
+public class SubqueryLimitUtils
+{
+ private static final double SUBQUERY_MEMORY_BYTES_FRACTION = 0.5;
+ private static final Logger log = new Logger(SubqueryLimitUtils.class);
+
+
+ public static final String UNLIMITED_LIMIT_VALUE = "unlimited";
+ public static final String AUTO_LIMIT_VALUE = "auto";
+
+ public static final Long UNLIMITED_LIMIT_REPRESENTATION = -1L;
+
+ private final long autoLimitBytes;
+
+ public SubqueryLimitUtils(
+ final LookupExtractorFactoryContainerProvider lookupManager,
+ final long maxMemoryInJvm,
+ final int brokerNumHttpConnections
+ )
+ {
+ autoLimitBytes = computeLimitBytesForAuto(lookupManager, maxMemoryInJvm, brokerNumHttpConnections);
+ }
+
+ public long convertSubqueryLimitStringToLong(final String maxSubqueryLimit)
+ {
+ if (UNLIMITED_LIMIT_VALUE.equalsIgnoreCase(maxSubqueryLimit)) {
+ return UNLIMITED_LIMIT_REPRESENTATION;
+ }
+ if (AUTO_LIMIT_VALUE.equalsIgnoreCase(maxSubqueryLimit)) {
+ return autoLimitBytes;
+ }
+
+ long retVal;
+ try {
+ retVal = Long.parseLong(maxSubqueryLimit);
+ }
+ catch (NumberFormatException e) {
+ throw InvalidInput.exception(
+ e,
+ "Unable to parse the provided maxSubqueryLimit [%s] to a valid number. Valid values for the "
+ + "maxSubqueryLimits can be 'auto', 'unlimited' or a positive number representing bytes to reserve.",
+ maxSubqueryLimit
+ );
+ }
+
+ // This can happen if the provided number is greater than Longs.MAX_VALUE
+ if (retVal < 0) {
+ throw InvalidInput.exception("Limit too large");
+ }
+
+ return retVal;
+ }
+
+ /**
+ * Computes the byte limit when 'auto' is passed as a parameter. This computes the total heap space available
+ * for the subquery inlining by getting a fraction of the total heap space in JVM, removing the size of the lookups,
+ * and dividing it by the maximum concurrent queries that can run. Maximum concurrent queries that Druid can
+ * run is usually limited by its broker's http threadpool size
+ */
+ private static long computeLimitBytesForAuto(
+ final LookupExtractorFactoryContainerProvider lookupManager,
+ final long maxMemoryInJvm,
+ final int brokerNumHttpConnections
+ )
+ {
+ long memoryInJvmWithoutLookups = maxMemoryInJvm - computeLookupFootprint(lookupManager);
+ long memoryInJvmForSubqueryResultsInlining = (long) (memoryInJvmWithoutLookups * SUBQUERY_MEMORY_BYTES_FRACTION);
+ long memoryInJvmForSubqueryResultsInliningPerQuery = memoryInJvmForSubqueryResultsInlining
+ / brokerNumHttpConnections;
+ return Math.max(memoryInJvmForSubqueryResultsInliningPerQuery, 1L);
+ }
+
+ /**
+ * Computes the size occupied by the lookups. If the size of the lookup cannot be computed, it skips over the lookup
+ */
+ private static long computeLookupFootprint(final LookupExtractorFactoryContainerProvider lookupManager)
+ {
+
+ if (lookupManager == null || lookupManager.getAllLookupNames() == null) {
+ log.warn("Failed to get the lookupManager for estimating lookup size. Skipping.");
+ return 0;
+ }
+
+ int lookupCount = 0;
+ long lookupFootprint = 0;
+
+ for (final String lookupName : lookupManager.getAllLookupNames()) {
+ final LookupExtractorFactoryContainer container = lookupManager.get(lookupName).orElse(null);
+
+ if (container != null) {
+ try {
+ final LookupExtractor extractor = container.getLookupExtractorFactory().get();
Review Comment:
Since MSQ is not time sensitive currently and the broker start up is we might need better logging here.
So here is what I propose.
Add an info log line mentioning the start time for this calculation and the end time for this calculation. Also mention in the log line that if this delta time is > 10 seconds, consider lowering the size of the lookups.
If there is a way to cache the size information on the lookup side that will be even cooler but I guess that can be a follow up PR to this one.
cc @pranavbhole who is working in the lookups area.
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Auto configuration for maxSubqueryBytes (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #14808:
URL: https://github.com/apache/druid/pull/14808#discussion_r1330317502
##########
server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java:
##########
@@ -127,6 +137,12 @@ public ClientQuerySegmentWalker(
this.serverConfig = serverConfig;
this.cache = cache;
this.cacheConfig = cacheConfig;
+ this.subqueryGuardrailHelper = new SubqueryGuardrailHelper(
+ lookupManager,
+ Runtime.getRuntime().maxMemory(),
+ httpClientConfig.getNumConnections()
Review Comment:
This is the # of connections from the Broker to Historicals. I think we should be using the # of queries, though? That's `ServerConfig#getNumThreads`.
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Auto configuration for maxSubqueryBytes (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla merged PR #14808:
URL: https://github.com/apache/druid/pull/14808
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Auto configuration for maxSubqueryBytes (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on PR #14808:
URL: https://github.com/apache/druid/pull/14808#issuecomment-1695071634
@abhishekagarwal87 Currently there is a limit per query in the broker guardrail. In the case of the example, let's say that overall 1/6th of broker memory is allocated for all the subqueries. The code would divide the 1/6th portion to the maximum number of queries that can be run on the system (limited to the number of broker threads), and each subquery has a local counter for the size.
With the global counter, I see that while it would allow more subqueries to pass in the happy case when no other is running, this can also potentially mean:
1. There would be resource contention amongst the various queries that are running on the system.
2. Oncoming queries can fail if the existing subqueries are hogging up the space, even if they don't require much space to materialize the results
3. In the worst case, subquery A can materialize some rows, subquery B can materialize some rows, and both fill up the shared space and then realize that none of them have enough space to continue, causing both queries to return resource limit exceeded.
While the above cases are slightly hyperbolized, we still would need to think about them before maintaining a global counter. Therefore "isolating" each query's available heap space seems to be better for the time moment, till we tackle the above. (FWIW, is also what the behavior with `maxSubqueryRows` 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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
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
Re: [PR] Auto configuration for maxSubqueryBytes (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #14808:
URL: https://github.com/apache/druid/pull/14808#discussion_r1330317502
##########
server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java:
##########
@@ -127,6 +137,12 @@ public ClientQuerySegmentWalker(
this.serverConfig = serverConfig;
this.cache = cache;
this.cacheConfig = cacheConfig;
+ this.subqueryGuardrailHelper = new SubqueryGuardrailHelper(
+ lookupManager,
+ Runtime.getRuntime().maxMemory(),
+ httpClientConfig.getNumConnections()
Review Comment:
This is the # of connections from the Broker to Historicals. I think we should be using the # of possible concurrent queries, though? That's `ServerConfig#getNumThreads`, a.k.a. `druid.server.http.numThreads`.
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Auto configuration for maxSubqueryBytes (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #14808:
URL: https://github.com/apache/druid/pull/14808#discussion_r1300994917
##########
server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java:
##########
@@ -127,6 +137,12 @@ public ClientQuerySegmentWalker(
this.serverConfig = serverConfig;
this.cache = cache;
this.cacheConfig = cacheConfig;
+ this.subqueryLimitUtils = new SubqueryLimitUtils(
Review Comment:
Generally utils donot have state ie only static methods. Does doing it this way aid testing ?
##########
server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java:
##########
@@ -114,7 +121,10 @@ public ClientQuerySegmentWalker(
ObjectMapper objectMapper,
ServerConfig serverConfig,
Cache cache,
- CacheConfig cacheConfig
+ CacheConfig cacheConfig,
+ LookupExtractorFactoryContainerProvider lookupManager,
+ DruidHttpClientConfig httpClientConfig,
+ SubqueryCountStatsProvider statsProvider
Review Comment:
```suggestion
SubqueryStatsProvider subQueryStatsProvider
```
##########
server/src/main/java/org/apache/druid/server/SubqueryLimitUtils.java:
##########
@@ -0,0 +1,132 @@
+/*
+ * 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 org.apache.druid.error.InvalidInput;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.query.lookup.LookupExtractor;
+import org.apache.druid.query.lookup.LookupExtractorFactoryContainer;
+import org.apache.druid.query.lookup.LookupExtractorFactoryContainerProvider;
+
+public class SubqueryLimitUtils
+{
+ private static final double SUBQUERY_MEMORY_BYTES_FRACTION = 0.5;
+ private static final Logger log = new Logger(SubqueryLimitUtils.class);
+
+
+ public static final String UNLIMITED_LIMIT_VALUE = "unlimited";
+ public static final String AUTO_LIMIT_VALUE = "auto";
+
+ public static final Long UNLIMITED_LIMIT_REPRESENTATION = -1L;
+
+ private final long autoLimitBytes;
+
+ public SubqueryLimitUtils(
+ final LookupExtractorFactoryContainerProvider lookupManager,
+ final long maxMemoryInJvm,
+ final int brokerNumHttpConnections
+ )
+ {
+ autoLimitBytes = computeLimitBytesForAuto(lookupManager, maxMemoryInJvm, brokerNumHttpConnections);
+ }
+
+ public long convertSubqueryLimitStringToLong(final String maxSubqueryLimit)
+ {
+ if (UNLIMITED_LIMIT_VALUE.equalsIgnoreCase(maxSubqueryLimit)) {
+ return UNLIMITED_LIMIT_REPRESENTATION;
+ }
+ if (AUTO_LIMIT_VALUE.equalsIgnoreCase(maxSubqueryLimit)) {
+ return autoLimitBytes;
+ }
+
+ long retVal;
+ try {
+ retVal = Long.parseLong(maxSubqueryLimit);
+ }
+ catch (NumberFormatException e) {
+ throw InvalidInput.exception(
+ e,
+ "Unable to parse the provided maxSubqueryLimit [%s] to a valid number. Valid values for the "
+ + "maxSubqueryLimits can be 'auto', 'unlimited' or a positive number representing bytes to reserve.",
+ maxSubqueryLimit
+ );
+ }
+
+ // This can happen if the provided number is greater than Longs.MAX_VALUE
+ if (retVal < 0) {
+ throw InvalidInput.exception("Limit too large");
+ }
+
+ return retVal;
+ }
+
+ /**
+ * Computes the byte limit when 'auto' is passed as a parameter. This computes the total heap space available
+ * for the subquery inlining by getting a fraction of the total heap space in JVM, removing the size of the lookups,
+ * and dividing it by the maximum concurrent queries that can run. Maximum concurrent queries that Druid can
+ * run is usually limited by its broker's http threadpool size
Review Comment:
Please add an example above
##########
server/src/main/java/org/apache/druid/server/metrics/SubqueryCountStatsMonitor.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.ImmutableMap;
+import com.google.inject.Inject;
+import org.apache.druid.java.util.emitter.service.ServiceEmitter;
+import org.apache.druid.java.util.emitter.service.ServiceMetricEvent;
+import org.apache.druid.java.util.metrics.AbstractMonitor;
+import org.apache.druid.java.util.metrics.KeyedDiff;
+
+import java.util.Map;
+
+public class SubqueryCountStatsMonitor extends AbstractMonitor
+{
+
+ private final KeyedDiff keyedDiff = new KeyedDiff();
+ private final SubqueryCountStatsProvider statsProvider;
+
+ @Inject
+ public SubqueryCountStatsMonitor(SubqueryCountStatsProvider statsProvider)
+ {
+ this.statsProvider = statsProvider;
+ }
+
+ @Override
+ public boolean doMonitor(ServiceEmitter emitter)
+ {
+ final ServiceMetricEvent.Builder builder = new ServiceMetricEvent.Builder();
+ final long subqueriesWithRowBasedLimit = statsProvider.subqueriesWithRowBasedLimit();
+ final long subqueriesWithByteBasedLimit = statsProvider.subqueriesWithByteBasedLimit();
+ final long subqueriesFallingBackToRowBasedLimit = statsProvider.subqueriesFallingBackToRowBasedLimit();
+ final long subqueriesFallingBackDueToUnsufficientTypeInfo = statsProvider.subqueriesFallingBackDueToUnsufficientTypeInfo();
+
+ Map<String, Long> diff = keyedDiff.to(
+ "subqueryCountStats",
+ ImmutableMap.of(
+ "subquery/rowLimit/count", subqueriesWithRowBasedLimit,
+ "subquery/byteLimit/count", subqueriesWithByteBasedLimit,
Review Comment:
Also Please document this in metrics.md
##########
server/src/main/java/org/apache/druid/server/metrics/SubqueryCountStatsMonitor.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.ImmutableMap;
+import com.google.inject.Inject;
+import org.apache.druid.java.util.emitter.service.ServiceEmitter;
+import org.apache.druid.java.util.emitter.service.ServiceMetricEvent;
+import org.apache.druid.java.util.metrics.AbstractMonitor;
+import org.apache.druid.java.util.metrics.KeyedDiff;
+
+import java.util.Map;
+
+public class SubqueryCountStatsMonitor extends AbstractMonitor
+{
+
+ private final KeyedDiff keyedDiff = new KeyedDiff();
+ private final SubqueryCountStatsProvider statsProvider;
+
+ @Inject
+ public SubqueryCountStatsMonitor(SubqueryCountStatsProvider statsProvider)
+ {
+ this.statsProvider = statsProvider;
+ }
+
+ @Override
+ public boolean doMonitor(ServiceEmitter emitter)
+ {
+ final ServiceMetricEvent.Builder builder = new ServiceMetricEvent.Builder();
+ final long subqueriesWithRowBasedLimit = statsProvider.subqueriesWithRowBasedLimit();
+ final long subqueriesWithByteBasedLimit = statsProvider.subqueriesWithByteBasedLimit();
+ final long subqueriesFallingBackToRowBasedLimit = statsProvider.subqueriesFallingBackToRowBasedLimit();
+ final long subqueriesFallingBackDueToUnsufficientTypeInfo = statsProvider.subqueriesFallingBackDueToUnsufficientTypeInfo();
+
+ Map<String, Long> diff = keyedDiff.to(
+ "subqueryCountStats",
+ ImmutableMap.of(
+ "subquery/rowLimit/count", subqueriesWithRowBasedLimit,
Review Comment:
Does this metric mean We have hit the row limit or we are using numRowBased guardrail is unclear from the name.
Since we are touching this code, we can also add the number of q's going over the limit but it can be a follow up PR as well.
##########
server/src/main/java/org/apache/druid/server/SubqueryLimitUtils.java:
##########
@@ -0,0 +1,132 @@
+/*
+ * 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 org.apache.druid.error.InvalidInput;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.query.lookup.LookupExtractor;
+import org.apache.druid.query.lookup.LookupExtractorFactoryContainer;
+import org.apache.druid.query.lookup.LookupExtractorFactoryContainerProvider;
+
+public class SubqueryLimitUtils
Review Comment:
Nit:: Instead of Limit lets use the word SubQueryGuardRail so the reader instantly figures out that this class is being used for adding guardRails to subQuery and not something like
`Select * from select * from foo limit 100`
##########
server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java:
##########
@@ -730,6 +763,11 @@ private static <T, QueryType extends Query<T>> Optional<DataSource> materializeR
useNestedForUnknownTypeInSubquery
);
}
+ catch (UnsupportedColumnTypeException e) {
+ statsProvider.incrementSubqueriesFallingBackDueToUnsufficientTypeInfo();
+ log.debug(e, "Type info in signature insufficient to materialize rows as frames.");
+ return Optional.empty();
+ }
catch (Exception e) {
log.debug(e, "Unable to materialize the results as frames due to an unhandleable exception "
Review Comment:
We should also add a unkownException to the statsProvider?
##########
extensions-contrib/moving-average-query/src/test/java/org/apache/druid/query/movingaverage/MovingAverageQueryTest.java:
##########
@@ -311,6 +312,17 @@ public void testQuery() throws IOException
Assert.assertNotNull(expectedResults);
Assert.assertThat(expectedResults, IsInstanceOf.instanceOf(List.class));
+ DruidHttpClientConfig httpClientConfig = new DruidHttpClientConfig()
+ {
+ @Override
+ public long getMaxQueuedBytes()
+ {
+ return 0L;
+ }
+
Review Comment:
Nit: extra spaces
##########
server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java:
##########
@@ -175,10 +197,13 @@ public <T> QueryRunner<T> getQueryRunnerForIntervals(Query<T> query, Iterable<In
final DataSource freeTradeDataSource = globalizeIfPossible(newQuery.getDataSource());
// do an inlining dry run to see if any inlining is necessary, without actually running the queries.
final int maxSubqueryRows = query.context().getMaxSubqueryRows(serverConfig.getMaxSubqueryRows());
- final long maxSubqueryMemory = query.context().getMaxSubqueryMemoryBytes(serverConfig.getMaxSubqueryBytes());
+ final String maxSubqueryMemoryString = query.context()
+ .getMaxSubqueryMemoryBytes(serverConfig.getMaxSubqueryBytes());
+ final long maxSubqueryMemory = subqueryLimitUtils.convertSubqueryLimitStringToLong(maxSubqueryMemoryString);
Review Comment:
202 and 200 line can be merged.
##########
server/src/main/java/org/apache/druid/server/SubqueryLimitUtils.java:
##########
@@ -0,0 +1,132 @@
+/*
+ * 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 org.apache.druid.error.InvalidInput;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.query.lookup.LookupExtractor;
+import org.apache.druid.query.lookup.LookupExtractorFactoryContainer;
+import org.apache.druid.query.lookup.LookupExtractorFactoryContainerProvider;
+
+public class SubqueryLimitUtils
+{
+ private static final double SUBQUERY_MEMORY_BYTES_FRACTION = 0.5;
+ private static final Logger log = new Logger(SubqueryLimitUtils.class);
+
+
+ public static final String UNLIMITED_LIMIT_VALUE = "unlimited";
+ public static final String AUTO_LIMIT_VALUE = "auto";
+
+ public static final Long UNLIMITED_LIMIT_REPRESENTATION = -1L;
+
+ private final long autoLimitBytes;
+
+ public SubqueryLimitUtils(
+ final LookupExtractorFactoryContainerProvider lookupManager,
+ final long maxMemoryInJvm,
+ final int brokerNumHttpConnections
+ )
+ {
+ autoLimitBytes = computeLimitBytesForAuto(lookupManager, maxMemoryInJvm, brokerNumHttpConnections);
+ }
+
+ public long convertSubqueryLimitStringToLong(final String maxSubqueryLimit)
+ {
+ if (UNLIMITED_LIMIT_VALUE.equalsIgnoreCase(maxSubqueryLimit)) {
+ return UNLIMITED_LIMIT_REPRESENTATION;
+ }
+ if (AUTO_LIMIT_VALUE.equalsIgnoreCase(maxSubqueryLimit)) {
+ return autoLimitBytes;
+ }
+
+ long retVal;
+ try {
+ retVal = Long.parseLong(maxSubqueryLimit);
+ }
+ catch (NumberFormatException e) {
+ throw InvalidInput.exception(
+ e,
+ "Unable to parse the provided maxSubqueryLimit [%s] to a valid number. Valid values for the "
+ + "maxSubqueryLimits can be 'auto', 'unlimited' or a positive number representing bytes to reserve.",
+ maxSubqueryLimit
+ );
+ }
+
+ // This can happen if the provided number is greater than Longs.MAX_VALUE
+ if (retVal < 0) {
+ throw InvalidInput.exception("Limit too large");
+ }
+
+ return retVal;
+ }
+
+ /**
+ * Computes the byte limit when 'auto' is passed as a parameter. This computes the total heap space available
+ * for the subquery inlining by getting a fraction of the total heap space in JVM, removing the size of the lookups,
+ * and dividing it by the maximum concurrent queries that can run. Maximum concurrent queries that Druid can
+ * run is usually limited by its broker's http threadpool size
+ */
+ private static long computeLimitBytesForAuto(
+ final LookupExtractorFactoryContainerProvider lookupManager,
+ final long maxMemoryInJvm,
+ final int brokerNumHttpConnections
+ )
+ {
+ long memoryInJvmWithoutLookups = maxMemoryInJvm - computeLookupFootprint(lookupManager);
+ long memoryInJvmForSubqueryResultsInlining = (long) (memoryInJvmWithoutLookups * SUBQUERY_MEMORY_BYTES_FRACTION);
+ long memoryInJvmForSubqueryResultsInliningPerQuery = memoryInJvmForSubqueryResultsInlining
+ / brokerNumHttpConnections;
+ return Math.max(memoryInJvmForSubqueryResultsInliningPerQuery, 1L);
+ }
+
+ /**
+ * Computes the size occupied by the lookups. If the size of the lookup cannot be computed, it skips over the lookup
+ */
+ private static long computeLookupFootprint(final LookupExtractorFactoryContainerProvider lookupManager)
+ {
+
+ if (lookupManager == null || lookupManager.getAllLookupNames() == null) {
+ log.warn("Failed to get the lookupManager for estimating lookup size. Skipping.");
+ return 0;
+ }
+
+ int lookupCount = 0;
+ long lookupFootprint = 0;
+
+ for (final String lookupName : lookupManager.getAllLookupNames()) {
+ final LookupExtractorFactoryContainer container = lookupManager.get(lookupName).orElse(null);
+
+ if (container != null) {
+ try {
+ final LookupExtractor extractor = container.getLookupExtractorFactory().get();
Review Comment:
Questions:
What ensures lookups are loaded completely.
`lookupFootprint += extractor.estimateHeapFootprint();` seems to be traversing each key value pair so it can become slow leading to slower broker startups times no ?
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Auto configuration for maxSubqueryBytes (druid)
Posted by "abhishekagarwal87 (via GitHub)" <gi...@apache.org>.
abhishekagarwal87 commented on PR #14808:
URL: https://github.com/apache/druid/pull/14808#issuecomment-1691733553
After this PR, will there be a global limit or the limit is still per query? It will be very useful to say that overall 1/6th of broker heap can be used for all subqueries. So a global counter is maintained and queries will fail if that limit is breached.
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Auto configuration for maxSubqueryBytes (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14808:
URL: https://github.com/apache/druid/pull/14808#discussion_r1306937152
##########
server/src/main/java/org/apache/druid/server/SubqueryLimitUtils.java:
##########
@@ -0,0 +1,132 @@
+/*
+ * 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 org.apache.druid.error.InvalidInput;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.query.lookup.LookupExtractor;
+import org.apache.druid.query.lookup.LookupExtractorFactoryContainer;
+import org.apache.druid.query.lookup.LookupExtractorFactoryContainerProvider;
+
+public class SubqueryLimitUtils
+{
+ private static final double SUBQUERY_MEMORY_BYTES_FRACTION = 0.5;
+ private static final Logger log = new Logger(SubqueryLimitUtils.class);
+
+
+ public static final String UNLIMITED_LIMIT_VALUE = "unlimited";
+ public static final String AUTO_LIMIT_VALUE = "auto";
+
+ public static final Long UNLIMITED_LIMIT_REPRESENTATION = -1L;
+
+ private final long autoLimitBytes;
+
+ public SubqueryLimitUtils(
+ final LookupExtractorFactoryContainerProvider lookupManager,
+ final long maxMemoryInJvm,
+ final int brokerNumHttpConnections
+ )
+ {
+ autoLimitBytes = computeLimitBytesForAuto(lookupManager, maxMemoryInJvm, brokerNumHttpConnections);
+ }
+
+ public long convertSubqueryLimitStringToLong(final String maxSubqueryLimit)
+ {
+ if (UNLIMITED_LIMIT_VALUE.equalsIgnoreCase(maxSubqueryLimit)) {
+ return UNLIMITED_LIMIT_REPRESENTATION;
+ }
+ if (AUTO_LIMIT_VALUE.equalsIgnoreCase(maxSubqueryLimit)) {
+ return autoLimitBytes;
+ }
+
+ long retVal;
+ try {
+ retVal = Long.parseLong(maxSubqueryLimit);
+ }
+ catch (NumberFormatException e) {
+ throw InvalidInput.exception(
+ e,
+ "Unable to parse the provided maxSubqueryLimit [%s] to a valid number. Valid values for the "
+ + "maxSubqueryLimits can be 'auto', 'unlimited' or a positive number representing bytes to reserve.",
+ maxSubqueryLimit
+ );
+ }
+
+ // This can happen if the provided number is greater than Longs.MAX_VALUE
+ if (retVal < 0) {
+ throw InvalidInput.exception("Limit too large");
+ }
+
+ return retVal;
+ }
+
+ /**
+ * Computes the byte limit when 'auto' is passed as a parameter. This computes the total heap space available
+ * for the subquery inlining by getting a fraction of the total heap space in JVM, removing the size of the lookups,
+ * and dividing it by the maximum concurrent queries that can run. Maximum concurrent queries that Druid can
+ * run is usually limited by its broker's http threadpool size
+ */
+ private static long computeLimitBytesForAuto(
+ final LookupExtractorFactoryContainerProvider lookupManager,
+ final long maxMemoryInJvm,
+ final int brokerNumHttpConnections
+ )
+ {
+ long memoryInJvmWithoutLookups = maxMemoryInJvm - computeLookupFootprint(lookupManager);
+ long memoryInJvmForSubqueryResultsInlining = (long) (memoryInJvmWithoutLookups * SUBQUERY_MEMORY_BYTES_FRACTION);
+ long memoryInJvmForSubqueryResultsInliningPerQuery = memoryInJvmForSubqueryResultsInlining
+ / brokerNumHttpConnections;
+ return Math.max(memoryInJvmForSubqueryResultsInliningPerQuery, 1L);
+ }
+
+ /**
+ * Computes the size occupied by the lookups. If the size of the lookup cannot be computed, it skips over the lookup
+ */
+ private static long computeLookupFootprint(final LookupExtractorFactoryContainerProvider lookupManager)
+ {
+
+ if (lookupManager == null || lookupManager.getAllLookupNames() == null) {
+ log.warn("Failed to get the lookupManager for estimating lookup size. Skipping.");
+ return 0;
+ }
+
+ int lookupCount = 0;
+ long lookupFootprint = 0;
+
+ for (final String lookupName : lookupManager.getAllLookupNames()) {
+ final LookupExtractorFactoryContainer container = lookupManager.get(lookupName).orElse(null);
+
+ if (container != null) {
+ try {
+ final LookupExtractor extractor = container.getLookupExtractorFactory().get();
Review Comment:
1. Lookups are usually loaded completely on the startup for querying processes. The behavior is handled by the flag - `enableLookupSyncOnStartup` which ensures that it is ideally set to true for the brokers.
2. Considering that a single entry takes about 100 bytes in the map table, a 100KB lookup would have about 1000 entries. Iterating over it a single time should be fine I think. For larger lookups, however, say unusual cases where the size of the lookup is over a limit, we can have some heuristics instead.
3. Since we do `estimateHeapFootprint` for every MSQ query, I think it should be relatively safe to include one time at the broker startup time. WDYT?
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Auto configuration for maxSubqueryBytes (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14808:
URL: https://github.com/apache/druid/pull/14808#discussion_r1306895775
##########
server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java:
##########
@@ -175,10 +197,13 @@ public <T> QueryRunner<T> getQueryRunnerForIntervals(Query<T> query, Iterable<In
final DataSource freeTradeDataSource = globalizeIfPossible(newQuery.getDataSource());
// do an inlining dry run to see if any inlining is necessary, without actually running the queries.
final int maxSubqueryRows = query.context().getMaxSubqueryRows(serverConfig.getMaxSubqueryRows());
- final long maxSubqueryMemory = query.context().getMaxSubqueryMemoryBytes(serverConfig.getMaxSubqueryBytes());
+ final String maxSubqueryMemoryString = query.context()
+ .getMaxSubqueryMemoryBytes(serverConfig.getMaxSubqueryBytes());
+ final long maxSubqueryMemory = subqueryLimitUtils.convertSubqueryLimitStringToLong(maxSubqueryMemoryString);
Review Comment:
Leaving this as is since merging the lines is causing the resulting declaration to be super long, and is slightly confusing 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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
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
Re: [PR] Auto configuration for maxSubqueryBytes (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on PR #14808:
URL: https://github.com/apache/druid/pull/14808#issuecomment-1707626321
Thanks for the comments @cryptoe @abhishekagarwal87.
--
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: commits-unsubscribe@druid.apache.org
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