This is an automated email from the ASF dual-hosted git repository.
gian pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/druid.git
The following commit(s) were added to refs/heads/master by this push:
new 6b6d73b5d44 Use min of scheduler threads and server threads for
subquery guardrails. (#15295)
6b6d73b5d44 is described below
commit 6b6d73b5d4447f21bbdacd7106fcd7c1c2c48d5d
Author: Gian Merlino <[email protected]>
AuthorDate: Wed Nov 1 22:34:53 2023 -0700
Use min of scheduler threads and server threads for subquery guardrails.
(#15295)
* Use min of scheduler threads and server threads for subquery guardrails.
This allows more memory to be used for subqueries when the query scheduler
is configured to limit queries below the number of server threads. The patch
also refactors the code so SubqueryGuardrailHelper is provided by a Guice
Provider rather than being created by ClientQuerySegmentWalker, to achieve
better separation of concerns.
* Exclude provider from coverage.
---
.../movingaverage/MovingAverageQueryTest.java | 4 +-
server/pom.xml | 2 +
.../druid/server/ClientQuerySegmentWalker.java | 13 ++---
.../druid/server/SubqueryGuardrailHelper.java | 10 ++--
.../server/SubqueryGuardrailHelperProvider.java | 68 ++++++++++++++++++++++
.../druid/server/ClientQuerySegmentWalkerTest.java | 3 +-
.../org/apache/druid/server/QueryStackTests.java | 6 +-
.../main/java/org/apache/druid/cli/CliBroker.java | 3 +
.../util/SpecificSegmentsQuerySegmentWalker.java | 3 +-
9 files changed, 90 insertions(+), 22 deletions(-)
diff --git
a/extensions-contrib/moving-average-query/src/test/java/org/apache/druid/query/movingaverage/MovingAverageQueryTest.java
b/extensions-contrib/moving-average-query/src/test/java/org/apache/druid/query/movingaverage/MovingAverageQueryTest.java
index 522e00a37d7..90dbcecf370 100644
---
a/extensions-contrib/moving-average-query/src/test/java/org/apache/druid/query/movingaverage/MovingAverageQueryTest.java
+++
b/extensions-contrib/moving-average-query/src/test/java/org/apache/druid/query/movingaverage/MovingAverageQueryTest.java
@@ -72,11 +72,13 @@ import
org.apache.druid.query.timeseries.TimeseriesResultValue;
import org.apache.druid.segment.join.MapJoinableFactory;
import org.apache.druid.server.ClientQuerySegmentWalker;
import org.apache.druid.server.QueryStackTests;
+import org.apache.druid.server.SubqueryGuardrailHelper;
import org.apache.druid.server.initialization.ServerConfig;
import org.apache.druid.server.metrics.NoopServiceEmitter;
import org.apache.druid.server.metrics.SubqueryCountStatsProvider;
import org.apache.druid.testing.InitializedNullHandlingTest;
import org.apache.druid.timeline.TimelineLookup;
+import org.apache.druid.utils.JvmUtils;
import org.hamcrest.core.IsInstanceOf;
import org.joda.time.Interval;
import org.junit.Assert;
@@ -388,7 +390,7 @@ public class MovingAverageQueryTest extends
InitializedNullHandlingTest
serverConfig,
null,
new CacheConfig(),
- null,
+ new SubqueryGuardrailHelper(null,
JvmUtils.getRuntimeInfo().getMaxHeapSizeBytes(), 1),
new SubqueryCountStatsProvider()
);
diff --git a/server/pom.xml b/server/pom.xml
index c2b8269cf2a..e3748b8c599 100644
--- a/server/pom.xml
+++ b/server/pom.xml
@@ -443,6 +443,8 @@
<!-- Tested in the SQL layer, but, oddly, not in this
module. -->
<exclude>org/apache/druid/server/QueryResponse.class</exclude>
<exclude>org/apache/druid/curator/CuratorModule.class</exclude>
+ <!-- Guice providers where unit tests would be
tightly-coupled and not useful -->
+
<exclude>org/apache/druid/server/SubqueryGuardrailHelperProvider.class</exclude>
</excludes>
</configuration>
</plugin>
diff --git
a/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java
b/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java
index 0e086881362..80c6d0a86af 100644
--- a/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java
+++ b/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java
@@ -58,7 +58,6 @@ import org.apache.druid.query.RetryQueryRunnerConfig;
import org.apache.druid.query.SegmentDescriptor;
import org.apache.druid.query.TableDataSource;
import org.apache.druid.query.context.ResponseContext;
-import org.apache.druid.query.lookup.LookupExtractorFactoryContainerProvider;
import org.apache.druid.query.planning.DataSourceAnalysis;
import org.apache.druid.segment.column.RowSignature;
import org.apache.druid.segment.join.JoinableFactory;
@@ -120,7 +119,7 @@ public class ClientQuerySegmentWalker implements
QuerySegmentWalker
ServerConfig serverConfig,
Cache cache,
CacheConfig cacheConfig,
- LookupExtractorFactoryContainerProvider lookupManager,
+ SubqueryGuardrailHelper subqueryGuardrailHelper,
SubqueryCountStatsProvider subqueryStatsProvider
)
{
@@ -134,11 +133,7 @@ public class ClientQuerySegmentWalker implements
QuerySegmentWalker
this.serverConfig = serverConfig;
this.cache = cache;
this.cacheConfig = cacheConfig;
- this.subqueryGuardrailHelper = new SubqueryGuardrailHelper(
- lookupManager,
- Runtime.getRuntime().maxMemory(),
- serverConfig.getNumThreads()
- );
+ this.subqueryGuardrailHelper = subqueryGuardrailHelper;
this.subqueryStatsProvider = subqueryStatsProvider;
}
@@ -154,7 +149,7 @@ public class ClientQuerySegmentWalker implements
QuerySegmentWalker
ServerConfig serverConfig,
Cache cache,
CacheConfig cacheConfig,
- LookupExtractorFactoryContainerProvider lookupManager,
+ SubqueryGuardrailHelper subqueryGuardrailHelper,
SubqueryCountStatsProvider subqueryStatsProvider
)
{
@@ -169,7 +164,7 @@ public class ClientQuerySegmentWalker implements
QuerySegmentWalker
serverConfig,
cache,
cacheConfig,
- lookupManager,
+ subqueryGuardrailHelper,
subqueryStatsProvider
);
}
diff --git
a/server/src/main/java/org/apache/druid/server/SubqueryGuardrailHelper.java
b/server/src/main/java/org/apache/druid/server/SubqueryGuardrailHelper.java
index 88845ef955e..faa97a802e6 100644
--- a/server/src/main/java/org/apache/druid/server/SubqueryGuardrailHelper.java
+++ b/server/src/main/java/org/apache/druid/server/SubqueryGuardrailHelper.java
@@ -48,11 +48,11 @@ public class SubqueryGuardrailHelper
public SubqueryGuardrailHelper(
final LookupExtractorFactoryContainerProvider lookupManager,
final long maxMemoryInJvm,
- final int brokerNumHttpConnections
+ final int maxConcurrentQueries
)
{
final DateTime start = DateTimes.nowUtc();
- autoLimitBytes = computeLimitBytesForAuto(lookupManager, maxMemoryInJvm,
brokerNumHttpConnections);
+ autoLimitBytes = computeLimitBytesForAuto(lookupManager, maxMemoryInJvm,
maxConcurrentQueries);
final long startupTimeMs = new Interval(start,
DateTimes.nowUtc()).toDurationMillis();
log.info("Took [%d] ms to initialize the SubqueryGuardrailHelper.",
startupTimeMs);
@@ -114,13 +114,13 @@ public class SubqueryGuardrailHelper
private static long computeLimitBytesForAuto(
final LookupExtractorFactoryContainerProvider lookupManager,
final long maxMemoryInJvm,
- final int brokerNumHttpConnections
+ final int maxConcurrentQueries
)
{
long memoryInJvmWithoutLookups = maxMemoryInJvm -
computeLookupFootprint(lookupManager);
long memoryInJvmForSubqueryResultsInlining = (long)
(memoryInJvmWithoutLookups * SUBQUERY_MEMORY_BYTES_FRACTION);
- long memoryInJvmForSubqueryResultsInliningPerQuery =
memoryInJvmForSubqueryResultsInlining
- /
brokerNumHttpConnections;
+ long memoryInJvmForSubqueryResultsInliningPerQuery =
+ memoryInJvmForSubqueryResultsInlining / maxConcurrentQueries;
return Math.max(memoryInJvmForSubqueryResultsInliningPerQuery, 1L);
}
diff --git
a/server/src/main/java/org/apache/druid/server/SubqueryGuardrailHelperProvider.java
b/server/src/main/java/org/apache/druid/server/SubqueryGuardrailHelperProvider.java
new file mode 100644
index 00000000000..ddbc0790084
--- /dev/null
+++
b/server/src/main/java/org/apache/druid/server/SubqueryGuardrailHelperProvider.java
@@ -0,0 +1,68 @@
+/*
+ * 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 com.google.inject.Provider;
+import org.apache.druid.guice.LazySingleton;
+import org.apache.druid.query.lookup.LookupExtractorFactoryContainerProvider;
+import org.apache.druid.server.initialization.ServerConfig;
+import org.apache.druid.utils.JvmUtils;
+
+public class SubqueryGuardrailHelperProvider implements
Provider<SubqueryGuardrailHelper>
+{
+ private final LookupExtractorFactoryContainerProvider
lookupExtractorFactoryContainerProvider;
+ private final ServerConfig serverConfig;
+ private final QuerySchedulerConfig querySchedulerConfig;
+
+ @Inject
+ public SubqueryGuardrailHelperProvider(
+ LookupExtractorFactoryContainerProvider
lookupExtractorFactoryContainerProvider,
+ ServerConfig serverConfig,
+ QuerySchedulerConfig querySchedulerConfig
+ )
+ {
+ this.lookupExtractorFactoryContainerProvider =
lookupExtractorFactoryContainerProvider;
+ this.serverConfig = serverConfig;
+ this.querySchedulerConfig = querySchedulerConfig;
+ }
+
+ @Override
+ @LazySingleton
+ public SubqueryGuardrailHelper get()
+ {
+ final int maxConcurrentQueries;
+
+ if (querySchedulerConfig.getNumThreads() > 0) {
+ maxConcurrentQueries = Math.min(
+ querySchedulerConfig.getNumThreads(),
+ serverConfig.getNumThreads()
+ );
+ } else {
+ maxConcurrentQueries = serverConfig.getNumThreads();
+ }
+
+ return new SubqueryGuardrailHelper(
+ lookupExtractorFactoryContainerProvider,
+ JvmUtils.getRuntimeInfo().getMaxHeapSizeBytes(),
+ maxConcurrentQueries
+ );
+ }
+}
diff --git
a/server/src/test/java/org/apache/druid/server/ClientQuerySegmentWalkerTest.java
b/server/src/test/java/org/apache/druid/server/ClientQuerySegmentWalkerTest.java
index 112275860e4..140b96da558 100644
---
a/server/src/test/java/org/apache/druid/server/ClientQuerySegmentWalkerTest.java
+++
b/server/src/test/java/org/apache/druid/server/ClientQuerySegmentWalkerTest.java
@@ -1507,8 +1507,7 @@ public class ClientQuerySegmentWalkerTest
),
conglomerate,
joinableFactory,
- serverConfig,
- null
+ serverConfig
);
}
diff --git a/server/src/test/java/org/apache/druid/server/QueryStackTests.java
b/server/src/test/java/org/apache/druid/server/QueryStackTests.java
index fd8839d9cab..2301a9ace39 100644
--- a/server/src/test/java/org/apache/druid/server/QueryStackTests.java
+++ b/server/src/test/java/org/apache/druid/server/QueryStackTests.java
@@ -83,6 +83,7 @@ import
org.apache.druid.server.metrics.SubqueryCountStatsProvider;
import org.apache.druid.server.scheduling.ManualQueryPrioritizationStrategy;
import org.apache.druid.server.scheduling.NoQueryLaningStrategy;
import org.apache.druid.timeline.VersionedIntervalTimeline;
+import org.apache.druid.utils.JvmUtils;
import org.junit.Assert;
import javax.annotation.Nullable;
@@ -117,8 +118,7 @@ public class QueryStackTests
final QuerySegmentWalker localWalker,
final QueryRunnerFactoryConglomerate conglomerate,
final JoinableFactory joinableFactory,
- final ServerConfig serverConfig,
- final LookupExtractorFactoryContainerProvider lookupManager
+ final ServerConfig serverConfig
)
{
return new ClientQuerySegmentWalker(
@@ -164,7 +164,7 @@ public class QueryStackTests
return false;
}
},
- lookupManager,
+ new SubqueryGuardrailHelper(null,
JvmUtils.getRuntimeInfo().getMaxHeapSizeBytes(), 1),
new SubqueryCountStatsProvider()
);
}
diff --git a/services/src/main/java/org/apache/druid/cli/CliBroker.java
b/services/src/main/java/org/apache/druid/cli/CliBroker.java
index 1de8a0f1d47..50adb9b9ef7 100644
--- a/services/src/main/java/org/apache/druid/cli/CliBroker.java
+++ b/services/src/main/java/org/apache/druid/cli/CliBroker.java
@@ -61,6 +61,8 @@ import org.apache.druid.server.ClientInfoResource;
import org.apache.druid.server.ClientQuerySegmentWalker;
import org.apache.druid.server.ResponseContextConfig;
import org.apache.druid.server.SegmentManager;
+import org.apache.druid.server.SubqueryGuardrailHelper;
+import org.apache.druid.server.SubqueryGuardrailHelperProvider;
import org.apache.druid.server.coordination.ServerType;
import org.apache.druid.server.coordination.ZkCoordinator;
import org.apache.druid.server.http.BrokerResource;
@@ -146,6 +148,7 @@ public class CliBroker extends ServerRunnable
binder.bind(BrokerQueryResource.class).in(LazySingleton.class);
Jerseys.addResource(binder, BrokerQueryResource.class);
+
binder.bind(SubqueryGuardrailHelper.class).toProvider(SubqueryGuardrailHelperProvider.class);
binder.bind(QueryCountStatsProvider.class).to(BrokerQueryResource.class).in(LazySingleton.class);
binder.bind(SubqueryCountStatsProvider.class).toInstance(new
SubqueryCountStatsProvider());
Jerseys.addResource(binder, BrokerResource.class);
diff --git
a/sql/src/test/java/org/apache/druid/sql/calcite/util/SpecificSegmentsQuerySegmentWalker.java
b/sql/src/test/java/org/apache/druid/sql/calcite/util/SpecificSegmentsQuerySegmentWalker.java
index a47bbcf9577..b43a6515956 100644
---
a/sql/src/test/java/org/apache/druid/sql/calcite/util/SpecificSegmentsQuerySegmentWalker.java
+++
b/sql/src/test/java/org/apache/druid/sql/calcite/util/SpecificSegmentsQuerySegmentWalker.java
@@ -116,8 +116,7 @@ public class SpecificSegmentsQuerySegmentWalker implements
QuerySegmentWalker, C
),
conglomerate,
joinableFactoryWrapper.getJoinableFactory(),
- new ServerConfig(),
- LOOKUP_EXTRACTOR_FACTORY_CONTAINER_PROVIDER
+ new ServerConfig()
);
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
