You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "vrajat (via GitHub)" <gi...@apache.org> on 2024/07/15 07:30:27 UTC
[PR] Instrument Multi-Stage Operator Execution to Track Resource Usage [pinot]
vrajat opened a new pull request, #13598:
URL: https://github.com/apache/pinot/pull/13598
OOM Protection in Pinot depends on accounting of resource usage of threads that execute a query. Resource accounting is currently supported for Single-Stage queries only. This PR adds instrumentation for Multi-Stage query engine as well. OOM protection is automatically available as resource usage is accounted in same data structures checked by the OOM protection thread.
Tags: multi-stage, observability
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1741404034
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadExecutionContext.java:
##########
@@ -23,6 +23,12 @@
*/
public interface ThreadExecutionContext {
+ public enum TaskType {
+ SSE, // Single Stage Engine
+ MSE, // Multi Stage Engine
+ UNKNOWN // Default
Review Comment:
Done
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1740952717
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/executor/OpChainSchedulerServiceTest.java:
##########
@@ -77,9 +78,10 @@ private OpChain getChain(MultiStageOperator operator) {
when(mailboxService.getHostname()).thenReturn("localhost");
when(mailboxService.getPort()).thenReturn(1234);
WorkerMetadata workerMetadata = new WorkerMetadata(0, ImmutableMap.of(), ImmutableMap.of());
+ ThreadExecutionContext parentContext = null;
OpChainExecutionContext context =
new OpChainExecutionContext(mailboxService, 123L, Long.MAX_VALUE, ImmutableMap.of(),
- new StageMetadata(0, ImmutableList.of(workerMetadata), ImmutableMap.of()), workerMetadata, null);
+ new StageMetadata(0, ImmutableList.of(workerMetadata), ImmutableMap.of()), workerMetadata, null, null);
Review Comment:
Similar to the other comment. Is OpChainExecutionContext._parentContext nullable or not?
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1741525816
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/pipeline/PipelineBreakerExecutor.java:
##########
@@ -68,7 +69,7 @@ private PipelineBreakerExecutor() {
@Nullable
public static PipelineBreakerResult executePipelineBreakers(OpChainSchedulerService scheduler,
MailboxService mailboxService, WorkerMetadata workerMetadata, StagePlan stagePlan,
- Map<String, String> opChainMetadata, long requestId, long deadlineMs) {
+ Map<String, String> opChainMetadata, long requestId, long deadlineMs, ThreadExecutionContext parentContext) {
Review Comment:
Made the change. This was useful in `PipelineBreakerTest` only. In other usages, a variable is passed through and cannot be removed.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1724426776
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java:
##########
@@ -286,17 +286,18 @@ public void setThreadResourceUsageProvider(ThreadResourceUsageProvider threadRes
}
@Override
- public void createExecutionContextInner(@Nullable String queryId, int taskId, @Nullable
+ public void createExecutionContextInner(@Nullable String queryId, int taskId,
+ ThreadExecutionContext.TaskType taskType, @Nullable
ThreadExecutionContext parentContext) {
_threadLocalEntry.get()._errorStatus.set(null);
if (parentContext == null) {
// is anchor thread
assert queryId != null;
- _threadLocalEntry.get().setThreadTaskStatus(queryId, CommonConstants.Accounting.ANCHOR_TASK_ID,
+ _threadLocalEntry.get().setThreadTaskStatus(queryId, CommonConstants.Accounting.ANCHOR_TASK_ID, taskType,
Thread.currentThread());
} else {
// not anchor thread
- _threadLocalEntry.get().setThreadTaskStatus(parentContext.getQueryId(), taskId,
+ _threadLocalEntry.get().setThreadTaskStatus(parentContext.getQueryId(), taskId, taskType,
Review Comment:
Done
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "jasperjiaguo (via GitHub)" <gi...@apache.org>.
jasperjiaguo commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1698049547
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/HashJoinOperator.java:
##########
@@ -260,6 +260,7 @@ private void buildBroadcastHashTable()
hashCollection.add(row);
Review Comment:
Have we considered the option to do `sampleAndCheckInterruptionPeriodically()` every X rows in this loop? Similar for some other operators. If the rightBlock size itself is large and the operation itself is mem-heavy (e.g. heap, hashmap add) the OOM may happen within a block if we do not sample at a finer granularity.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1741524669
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/OpChainExecutionContext.java:
##########
@@ -123,4 +126,8 @@ public ServerPlanRequestContext getLeafStageContext() {
public void setLeafStageContext(ServerPlanRequestContext leafStageContext) {
_leafStageContext = leafStageContext;
}
+
+ public ThreadExecutionContext getParentContext() {
Review Comment:
Done.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1741483574
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/OpChainExecutionContext.java:
##########
@@ -123,4 +126,8 @@ public ServerPlanRequestContext getLeafStageContext() {
public void setLeafStageContext(ServerPlanRequestContext leafStageContext) {
_leafStageContext = leafStageContext;
}
+
+ public ThreadExecutionContext getParentContext() {
Review Comment:
This method and the attribute should also be nullable.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "jasperjiaguo (via GitHub)" <gi...@apache.org>.
jasperjiaguo commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1698049547
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/HashJoinOperator.java:
##########
@@ -260,6 +260,7 @@ private void buildBroadcastHashTable()
hashCollection.add(row);
Review Comment:
Have we considered the option to do sampleAndCheckInterruption every X rows in this loop? Similar for some other operators. If the rightBlock size itself is large and the operation itself is mem-heavy (e.g. heap, hashmap add) the OOM may happen within a block if we do not sample at a finer granularity.
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java:
##########
@@ -286,17 +286,18 @@ public void setThreadResourceUsageProvider(ThreadResourceUsageProvider threadRes
}
@Override
- public void createExecutionContextInner(@Nullable String queryId, int taskId, @Nullable
+ public void createExecutionContextInner(@Nullable String queryId, int taskId,
+ ThreadExecutionContext.TaskType taskType, @Nullable
ThreadExecutionContext parentContext) {
_threadLocalEntry.get()._errorStatus.set(null);
if (parentContext == null) {
// is anchor thread
assert queryId != null;
- _threadLocalEntry.get().setThreadTaskStatus(queryId, CommonConstants.Accounting.ANCHOR_TASK_ID,
+ _threadLocalEntry.get().setThreadTaskStatus(queryId, CommonConstants.Accounting.ANCHOR_TASK_ID, taskType,
Thread.currentThread());
} else {
// not anchor thread
- _threadLocalEntry.get().setThreadTaskStatus(parentContext.getQueryId(), taskId,
+ _threadLocalEntry.get().setThreadTaskStatus(parentContext.getQueryId(), taskId, taskType,
Review Comment:
Should we set this from the anchor thread via `parentContext.getTaskType()`?
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1740951160
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/executor/OpChainSchedulerServiceTest.java:
##########
@@ -77,9 +78,10 @@ private OpChain getChain(MultiStageOperator operator) {
when(mailboxService.getHostname()).thenReturn("localhost");
when(mailboxService.getPort()).thenReturn(1234);
WorkerMetadata workerMetadata = new WorkerMetadata(0, ImmutableMap.of(), ImmutableMap.of());
+ ThreadExecutionContext parentContext = null;
Review Comment:
Is this variable used?
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "jasperjiaguo (via GitHub)" <gi...@apache.org>.
jasperjiaguo commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1703634796
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/operator/MultiStageAccountingTest.java:
##########
@@ -0,0 +1,246 @@
+/**
Review Comment:
do we want to add a test case of query being killed in MSE execution?
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1716278467
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/HashJoinOperator.java:
##########
@@ -260,6 +260,7 @@ private void buildBroadcastHashTable()
hashCollection.add(row);
Review Comment:
I did look into that. I chose to check for every block for a couple of reasons.
- The block size is ~10000 rows which is very similar to the constant - 8192.
- It was easier to call this function after every block and not worry about specific if-else & try-catch in every implementation.
In this specific case too, the hash table is being built and sample/interruption will run every 10000 rows.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1740950275
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/QueryServerEnclosure.java:
##########
@@ -112,7 +112,8 @@ public void shutDown() {
public CompletableFuture<Void> processQuery(WorkerMetadata workerMetadata, StagePlan stagePlan,
Map<String, String> requestMetadataMap) {
- return CompletableFuture.runAsync(() -> _queryRunner.processQuery(workerMetadata, stagePlan, requestMetadataMap),
+ return CompletableFuture.runAsync(
+ () -> _queryRunner.processQuery(workerMetadata, stagePlan, requestMetadataMap, null),
Review Comment:
QueryRunner.processQuery last argument is not annotated with Nullable so we should not call it with null. We need to either change the QueryRunner method signature or provide a value here.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1741406679
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/HashJoinOperator.java:
##########
@@ -260,6 +260,7 @@ private void buildBroadcastHashTable()
hashCollection.add(row);
}
_currentRowsInHashTable += container.size();
+ sampleAndCheckInterruption();
Review Comment:
Done. I am calling the function in `buildJoinDataBlock` as well.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1740954643
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/operator/MultiStageAccountingTest.java:
##########
@@ -0,0 +1,246 @@
+/**
+ * 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.pinot.query.runtime.operator;
+
+import com.google.common.collect.ImmutableList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.List;
+import org.apache.calcite.rel.RelFieldCollation;
+import org.apache.calcite.rel.core.JoinRelType;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.log4j.Level;
+import org.apache.log4j.LogManager;
+import org.apache.pinot.common.datablock.DataBlock;
+import org.apache.pinot.common.metrics.ServerMetrics;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.core.accounting.PerQueryCPUMemAccountantFactory;
+import org.apache.pinot.query.planner.logical.RexExpression;
+import org.apache.pinot.query.planner.plannode.AggregateNode;
+import org.apache.pinot.query.planner.plannode.JoinNode;
+import org.apache.pinot.query.planner.plannode.PlanNode;
+import org.apache.pinot.query.planner.plannode.SortNode;
+import org.apache.pinot.query.planner.plannode.WindowNode;
+import org.apache.pinot.query.routing.VirtualServerAddress;
+import org.apache.pinot.query.runtime.blocks.TransferableBlock;
+import org.apache.pinot.query.runtime.blocks.TransferableBlockTestUtils;
+import org.apache.pinot.spi.accounting.QueryResourceTracker;
+import org.apache.pinot.spi.accounting.ThreadExecutionContext;
+import org.apache.pinot.spi.accounting.ThreadResourceTracker;
+import org.apache.pinot.spi.accounting.ThreadResourceUsageAccountant;
+import org.apache.pinot.spi.accounting.ThreadResourceUsageProvider;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.trace.Tracing;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.testng.ITest;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Factory;
+import org.testng.annotations.Test;
+
+import static org.apache.pinot.common.utils.DataSchema.ColumnDataType.DOUBLE;
+import static org.apache.pinot.common.utils.DataSchema.ColumnDataType.INT;
+import static org.mockito.Mockito.when;
+import static org.mockito.MockitoAnnotations.openMocks;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
+
+
+public class MultiStageAccountingTest implements ITest {
+ private AutoCloseable _mocks;
+ @Mock
+ private VirtualServerAddress _serverAddress;
+
+ protected String _testName;
+ protected MultiStageOperator _operator;
+
+ @Factory(dataProvider = "operatorProvider")
+ public MultiStageAccountingTest(String testName, MultiStageOperator operator) {
+ _testName = testName;
+ _operator = operator;
+ }
+
+ @BeforeClass
+ public static void setUpClass() {
+ // set up logging and configs
+ LogManager.getLogger(PerQueryCPUMemAccountantFactory.PerQueryCPUMemResourceUsageAccountant.class)
+ .setLevel(Level.OFF);
Review Comment:
I guess this is turning logs off? Why do we want to do that? I would expect these logs to be useful trying to find the issues detected by the tests.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Initialize Resource Usage Accountant in Multi-Stage Operators [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #13598:
URL: https://github.com/apache/pinot/pull/13598#issuecomment-2228199119
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/13598?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `0%` with `17 lines` in your changes missing coverage. Please review.
> Project coverage is 0.00%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`69dacb4`)](https://app.codecov.io/gh/apache/pinot/commit/69dacb463d9196f2a26ff82bddff53d0c0aa65f2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 750 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/13598?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...uery/runtime/executor/OpChainSchedulerService.java](https://app.codecov.io/gh/apache/pinot/pull/13598?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Fexecutor%2FOpChainSchedulerService.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9leGVjdXRvci9PcENoYWluU2NoZWR1bGVyU2VydmljZS5qYXZh) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ot/query/runtime/plan/OpChainExecutionContext.java](https://app.codecov.io/gh/apache/pinot/pull/13598?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Fplan%2FOpChainExecutionContext.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9wbGFuL09wQ2hhaW5FeGVjdXRpb25Db250ZXh0LmphdmE=) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...apache/pinot/query/service/server/QueryServer.java](https://app.codecov.io/gh/apache/pinot/pull/13598?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fservice%2Fserver%2FQueryServer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvc2VydmljZS9zZXJ2ZXIvUXVlcnlTZXJ2ZXIuamF2YQ==) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...g/apache/pinot/query/runtime/operator/OpChain.java](https://app.codecov.io/gh/apache/pinot/pull/13598?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Foperator%2FOpChain.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9PcENoYWluLmphdmE=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../pinot/query/service/dispatch/QueryDispatcher.java](https://app.codecov.io/gh/apache/pinot/pull/13598?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fservice%2Fdispatch%2FQueryDispatcher.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvc2VydmljZS9kaXNwYXRjaC9RdWVyeURpc3BhdGNoZXIuamF2YQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...requesthandler/MultiStageBrokerRequestHandler.java](https://app.codecov.io/gh/apache/pinot/pull/13598?src=pr&el=tree&filepath=pinot-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fbroker%2Frequesthandler%2FMultiStageBrokerRequestHandler.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvTXVsdGlTdGFnZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...apache/pinot/query/runtime/operator/OpChainId.java](https://app.codecov.io/gh/apache/pinot/pull/13598?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Foperator%2FOpChainId.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9PcENoYWluSWQuamF2YQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...runtime/plan/pipeline/PipelineBreakerExecutor.java](https://app.codecov.io/gh/apache/pinot/pull/13598?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Fplan%2Fpipeline%2FPipelineBreakerExecutor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9wbGFuL3BpcGVsaW5lL1BpcGVsaW5lQnJlYWtlckV4ZWN1dG9yLmphdmE=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (59551e4) and HEAD (69dacb4). Click for more details.
>
> <details><summary>HEAD has 53 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (69dacb4) |
>|------|------|------|
>|integration|7|1|
>|integration2|3|1|
>|temurin|12|1|
>|java-21|7|1|
>|skip-bytebuffers-true|3|1|
>|skip-bytebuffers-false|7|0|
>|unittests|5|0|
>|unittests1|2|0|
>|java-11|5|0|
>|unittests2|3|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #13598 +/- ##
=============================================
- Coverage 61.75% 0.00% -61.76%
=============================================
Files 2436 2480 +44
Lines 133233 136955 +3722
Branches 20636 21263 +627
=============================================
- Hits 82274 0 -82274
- Misses 44911 136955 +92044
+ Partials 6048 0 -6048
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/13598/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/13598/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/13598/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/13598/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/13598/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/13598/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/13598/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.63%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/13598/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/13598/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-27.73%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/13598/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.76%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/13598/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/13598/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/13598/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/13598?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1740960127
##########
pinot-spi/src/main/java/org/apache/pinot/spi/trace/Tracing.java:
##########
@@ -254,14 +260,25 @@ private ThreadAccountantOps() {
}
public static void setupRunner(String queryId) {
+ setupRunner(queryId, ThreadExecutionContext.TaskType.SSE);
+ }
+
+ public static void setupRunner(String queryId, ThreadExecutionContext.TaskType taskType) {
Tracing.getThreadAccountant().setThreadResourceUsageProvider(new ThreadResourceUsageProvider());
- Tracing.getThreadAccountant().createExecutionContext(queryId, CommonConstants.Accounting.ANCHOR_TASK_ID, null);
+ Tracing.getThreadAccountant()
+ .createExecutionContext(queryId, CommonConstants.Accounting.ANCHOR_TASK_ID, taskType, null);
}
public static void setupWorker(int taskId, ThreadResourceUsageProvider threadResourceUsageProvider,
ThreadExecutionContext threadExecutionContext) {
+ setupWorker(taskId, ThreadExecutionContext.TaskType.SSE, threadResourceUsageProvider, threadExecutionContext);
+ }
+
+ public static void setupWorker(int taskId, ThreadExecutionContext.TaskType taskType,
Review Comment:
Can we add some javadoc saying explaining these methods? Specially saying that the one without TaskType defaults to SSE.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on PR #13598:
URL: https://github.com/apache/pinot/pull/13598#issuecomment-2325991568
Test failure is unrelated. Looks like a flaky test
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1716278736
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/operator/MultiStageAccountingTest.java:
##########
@@ -0,0 +1,246 @@
+/**
Review Comment:
Since the trheading model is different, I couldnt make those tests work. I'll give it a try again.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1716292146
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java:
##########
@@ -286,17 +286,18 @@ public void setThreadResourceUsageProvider(ThreadResourceUsageProvider threadRes
}
@Override
- public void createExecutionContextInner(@Nullable String queryId, int taskId, @Nullable
+ public void createExecutionContextInner(@Nullable String queryId, int taskId,
+ ThreadExecutionContext.TaskType taskType, @Nullable
ThreadExecutionContext parentContext) {
_threadLocalEntry.get()._errorStatus.set(null);
if (parentContext == null) {
// is anchor thread
assert queryId != null;
- _threadLocalEntry.get().setThreadTaskStatus(queryId, CommonConstants.Accounting.ANCHOR_TASK_ID,
+ _threadLocalEntry.get().setThreadTaskStatus(queryId, CommonConstants.Accounting.ANCHOR_TASK_ID, taskType,
Thread.currentThread());
} else {
// not anchor thread
- _threadLocalEntry.get().setThreadTaskStatus(parentContext.getQueryId(), taskId,
+ _threadLocalEntry.get().setThreadTaskStatus(parentContext.getQueryId(), taskId, taskType,
Review Comment:
I'll give this a try
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #13598:
URL: https://github.com/apache/pinot/pull/13598
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1741406334
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/QueryServerEnclosure.java:
##########
@@ -112,7 +112,8 @@ public void shutDown() {
public CompletableFuture<Void> processQuery(WorkerMetadata workerMetadata, StagePlan stagePlan,
Map<String, String> requestMetadataMap) {
- return CompletableFuture.runAsync(() -> _queryRunner.processQuery(workerMetadata, stagePlan, requestMetadataMap),
+ return CompletableFuture.runAsync(
+ () -> _queryRunner.processQuery(workerMetadata, stagePlan, requestMetadataMap, null),
Review Comment:
Done
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/SetOperator.java:
##########
@@ -119,6 +119,7 @@ protected void constructRightBlockSet() {
_rightRowSet.add(new Record(row));
}
}
+ sampleAndCheckInterruption();
Review Comment:
Done
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1741486387
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/pipeline/PipelineBreakerExecutor.java:
##########
@@ -68,7 +69,7 @@ private PipelineBreakerExecutor() {
@Nullable
public static PipelineBreakerResult executePipelineBreakers(OpChainSchedulerService scheduler,
MailboxService mailboxService, WorkerMetadata workerMetadata, StagePlan stagePlan,
- Map<String, String> opChainMetadata, long requestId, long deadlineMs) {
+ Map<String, String> opChainMetadata, long requestId, long deadlineMs, ThreadExecutionContext parentContext) {
Review Comment:
parentContext should be nullable.
nit/suggestion: Can we have a executePipelineBreaker(OpChainSchedulerService scheduler,
MailboxService mailboxService, WorkerMetadata workerMetadata, StagePlan stagePlan,
Map<String, String> opChainMetadata, long requestId, long deadlineMs) that just calls this method with null as last argument? That would reduce this PR by a large margin.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1740938387
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/HashJoinOperator.java:
##########
@@ -260,6 +260,7 @@ private void buildBroadcastHashTable()
hashCollection.add(row);
}
_currentRowsInHashTable += container.size();
+ sampleAndCheckInterruption();
Review Comment:
IICU We are only calling `sampleAndChekInterruption` when the hash table is being build, but we should also call it whenever new blocks are received from the left hand side. Specifically, we may be creating very large maps when using cross join functions, but anyway the correct thing would be to call this method on every join type whenever a block is being processed
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1740945995
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/SetOperator.java:
##########
@@ -119,6 +119,7 @@ protected void constructRightBlockSet() {
_rightRowSet.add(new Record(row));
}
}
+ sampleAndCheckInterruption();
Review Comment:
I think we should also have to call this method during `constructResultBlockSet`
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1740957223
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadExecutionContext.java:
##########
@@ -23,6 +23,12 @@
*/
public interface ThreadExecutionContext {
+ public enum TaskType {
+ SSE, // Single Stage Engine
+ MSE, // Multi Stage Engine
+ UNKNOWN // Default
Review Comment:
nit: Comments are great, but that would be better if they were included as javadoc, so IDE users can get the same comment without having to download (and open) the sources
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1741402995
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/operator/MultiStageAccountingTest.java:
##########
@@ -0,0 +1,246 @@
+/**
+ * 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.pinot.query.runtime.operator;
+
+import com.google.common.collect.ImmutableList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.List;
+import org.apache.calcite.rel.RelFieldCollation;
+import org.apache.calcite.rel.core.JoinRelType;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.log4j.Level;
+import org.apache.log4j.LogManager;
+import org.apache.pinot.common.datablock.DataBlock;
+import org.apache.pinot.common.metrics.ServerMetrics;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.core.accounting.PerQueryCPUMemAccountantFactory;
+import org.apache.pinot.query.planner.logical.RexExpression;
+import org.apache.pinot.query.planner.plannode.AggregateNode;
+import org.apache.pinot.query.planner.plannode.JoinNode;
+import org.apache.pinot.query.planner.plannode.PlanNode;
+import org.apache.pinot.query.planner.plannode.SortNode;
+import org.apache.pinot.query.planner.plannode.WindowNode;
+import org.apache.pinot.query.routing.VirtualServerAddress;
+import org.apache.pinot.query.runtime.blocks.TransferableBlock;
+import org.apache.pinot.query.runtime.blocks.TransferableBlockTestUtils;
+import org.apache.pinot.spi.accounting.QueryResourceTracker;
+import org.apache.pinot.spi.accounting.ThreadExecutionContext;
+import org.apache.pinot.spi.accounting.ThreadResourceTracker;
+import org.apache.pinot.spi.accounting.ThreadResourceUsageAccountant;
+import org.apache.pinot.spi.accounting.ThreadResourceUsageProvider;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.trace.Tracing;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.testng.ITest;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Factory;
+import org.testng.annotations.Test;
+
+import static org.apache.pinot.common.utils.DataSchema.ColumnDataType.DOUBLE;
+import static org.apache.pinot.common.utils.DataSchema.ColumnDataType.INT;
+import static org.mockito.Mockito.when;
+import static org.mockito.MockitoAnnotations.openMocks;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
+
+
+public class MultiStageAccountingTest implements ITest {
+ private AutoCloseable _mocks;
+ @Mock
+ private VirtualServerAddress _serverAddress;
+
+ protected String _testName;
+ protected MultiStageOperator _operator;
+
+ @Factory(dataProvider = "operatorProvider")
+ public MultiStageAccountingTest(String testName, MultiStageOperator operator) {
+ _testName = testName;
+ _operator = operator;
+ }
+
+ @BeforeClass
+ public static void setUpClass() {
+ // set up logging and configs
+ LogManager.getLogger(PerQueryCPUMemAccountantFactory.PerQueryCPUMemResourceUsageAccountant.class)
+ .setLevel(Level.OFF);
Review Comment:
This is copy/paste from the V1 test. I ran it without turning it off and the logs were not verbose. I'll remove these lines.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1741401444
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/executor/OpChainSchedulerServiceTest.java:
##########
@@ -77,9 +78,10 @@ private OpChain getChain(MultiStageOperator operator) {
when(mailboxService.getHostname()).thenReturn("localhost");
when(mailboxService.getPort()).thenReturn(1234);
WorkerMetadata workerMetadata = new WorkerMetadata(0, ImmutableMap.of(), ImmutableMap.of());
+ ThreadExecutionContext parentContext = null;
Review Comment:
Removed.
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/executor/OpChainSchedulerServiceTest.java:
##########
@@ -77,9 +78,10 @@ private OpChain getChain(MultiStageOperator operator) {
when(mailboxService.getHostname()).thenReturn("localhost");
when(mailboxService.getPort()).thenReturn(1234);
WorkerMetadata workerMetadata = new WorkerMetadata(0, ImmutableMap.of(), ImmutableMap.of());
+ ThreadExecutionContext parentContext = null;
OpChainExecutionContext context =
new OpChainExecutionContext(mailboxService, 123L, Long.MAX_VALUE, ImmutableMap.of(),
- new StageMetadata(0, ImmutableList.of(workerMetadata), ImmutableMap.of()), workerMetadata, null);
+ new StageMetadata(0, ImmutableList.of(workerMetadata), ImmutableMap.of()), workerMetadata, null, null);
Review Comment:
Done
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1741406251
##########
pinot-spi/src/main/java/org/apache/pinot/spi/trace/Tracing.java:
##########
@@ -254,14 +260,25 @@ private ThreadAccountantOps() {
}
public static void setupRunner(String queryId) {
+ setupRunner(queryId, ThreadExecutionContext.TaskType.SSE);
+ }
+
+ public static void setupRunner(String queryId, ThreadExecutionContext.TaskType taskType) {
Tracing.getThreadAccountant().setThreadResourceUsageProvider(new ThreadResourceUsageProvider());
- Tracing.getThreadAccountant().createExecutionContext(queryId, CommonConstants.Accounting.ANCHOR_TASK_ID, null);
+ Tracing.getThreadAccountant()
+ .createExecutionContext(queryId, CommonConstants.Accounting.ANCHOR_TASK_ID, taskType, null);
}
public static void setupWorker(int taskId, ThreadResourceUsageProvider threadResourceUsageProvider,
ThreadExecutionContext threadExecutionContext) {
+ setupWorker(taskId, ThreadExecutionContext.TaskType.SSE, threadResourceUsageProvider, threadExecutionContext);
+ }
+
+ public static void setupWorker(int taskId, ThreadExecutionContext.TaskType taskType,
Review Comment:
Done
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add OOM Protection Support for Multi-Stage Queries [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13598:
URL: https://github.com/apache/pinot/pull/13598#discussion_r1741482743
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/operator/MultiStageAccountingTest.java:
##########
@@ -0,0 +1,246 @@
+/**
+ * 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.pinot.query.runtime.operator;
+
+import com.google.common.collect.ImmutableList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.List;
+import org.apache.calcite.rel.RelFieldCollation;
+import org.apache.calcite.rel.core.JoinRelType;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.log4j.Level;
+import org.apache.log4j.LogManager;
+import org.apache.pinot.common.datablock.DataBlock;
+import org.apache.pinot.common.metrics.ServerMetrics;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.core.accounting.PerQueryCPUMemAccountantFactory;
+import org.apache.pinot.query.planner.logical.RexExpression;
+import org.apache.pinot.query.planner.plannode.AggregateNode;
+import org.apache.pinot.query.planner.plannode.JoinNode;
+import org.apache.pinot.query.planner.plannode.PlanNode;
+import org.apache.pinot.query.planner.plannode.SortNode;
+import org.apache.pinot.query.planner.plannode.WindowNode;
+import org.apache.pinot.query.routing.VirtualServerAddress;
+import org.apache.pinot.query.runtime.blocks.TransferableBlock;
+import org.apache.pinot.query.runtime.blocks.TransferableBlockTestUtils;
+import org.apache.pinot.spi.accounting.QueryResourceTracker;
+import org.apache.pinot.spi.accounting.ThreadExecutionContext;
+import org.apache.pinot.spi.accounting.ThreadResourceTracker;
+import org.apache.pinot.spi.accounting.ThreadResourceUsageAccountant;
+import org.apache.pinot.spi.accounting.ThreadResourceUsageProvider;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.trace.Tracing;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.testng.ITest;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Factory;
+import org.testng.annotations.Test;
+
+import static org.apache.pinot.common.utils.DataSchema.ColumnDataType.DOUBLE;
+import static org.apache.pinot.common.utils.DataSchema.ColumnDataType.INT;
+import static org.mockito.Mockito.when;
+import static org.mockito.MockitoAnnotations.openMocks;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
+
+
+public class MultiStageAccountingTest implements ITest {
+ private AutoCloseable _mocks;
+ @Mock
+ private VirtualServerAddress _serverAddress;
+
+ protected String _testName;
+ protected MultiStageOperator _operator;
+
+ @Factory(dataProvider = "operatorProvider")
+ public MultiStageAccountingTest(String testName, MultiStageOperator operator) {
+ _testName = testName;
+ _operator = operator;
+ }
+
+ @BeforeClass
+ public static void setUpClass() {
+ // set up logging and configs
+ LogManager.getLogger(PerQueryCPUMemAccountantFactory.PerQueryCPUMemResourceUsageAccountant.class)
+ .setLevel(Level.OFF);
Review Comment:
I guess it is fine to run with logs off, it was just strange to see that :laughing:
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org