You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "adarshsanjeev (via GitHub)" <gi...@apache.org> on 2023/04/11 04:38:18 UTC
[GitHub] [druid] adarshsanjeev opened a new pull request, #14061: Move MSQ temporary storage to a runtime parameter instead of being configured from query context
adarshsanjeev opened a new pull request, #14061:
URL: https://github.com/apache/druid/pull/14061
Currently, the user has to configure `composedIntermediateSuperSorterStorageEnabled` and `intermediateSuperSorterStorageMaxLocalBytes` for each query. However, these are generally not something that users would be able to decide. This has been moved to a runtime parameter, so that they can be configured by the manager of the cluster. This would make reduce the number of potential configurations a user has to make.
<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
[GitHub] [druid] cryptoe commented on a diff in pull request #14061: Move MSQ temporary storage to a runtime parameter instead of being configured from query context
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #14061:
URL: https://github.com/apache/druid/pull/14061#discussion_r1162300480
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java:
##########
@@ -588,21 +588,20 @@ private QueryDefinition initializeQueryDefAndState(final Closer closer)
.orElse(MSQWarnings.DEFAULT_MAX_PARSE_EXCEPTIONS_ALLOWED);
}
+ WorkerStorageParameters workerStorageParameters =
+ WorkerStorageParameters.createProductionInstanceForController(context.injector(), isDurableStorageEnabled);
+
ImmutableMap.Builder<String, Object> taskContextOverridesBuilder = ImmutableMap.builder();
taskContextOverridesBuilder
.put(
MultiStageQueryContext.CTX_DURABLE_SHUFFLE_STORAGE,
isDurableStorageEnabled
).put(
MultiStageQueryContext.CTX_COMPOSED_INTERMEDIATE_SUPER_SORTER_STORAGE,
Review Comment:
Should we remove this parameter ?
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/config/TaskConfig.java:
##########
@@ -78,6 +78,7 @@
private static final Period DEFAULT_DIRECTORY_LOCK_TIMEOUT = new Period("PT10M");
private static final Period DEFAULT_GRACEFUL_SHUTDOWN_TIMEOUT = new Period("PT5M");
private static final boolean DEFAULT_STORE_EMPTY_COLUMNS = true;
+ private static final long DEFAULT_TMP_STORAGE_BYTES_PER_TASK = Long.MAX_VALUE; // TODO: check!!!
Review Comment:
Is this a left over todo ?
Where is the property `tmpStorageBytesPerTask` documented ?
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java:
##########
@@ -588,21 +588,20 @@ private QueryDefinition initializeQueryDefAndState(final Closer closer)
.orElse(MSQWarnings.DEFAULT_MAX_PARSE_EXCEPTIONS_ALLOWED);
}
+ WorkerStorageParameters workerStorageParameters =
+ WorkerStorageParameters.createProductionInstanceForController(context.injector(), isDurableStorageEnabled);
+
ImmutableMap.Builder<String, Object> taskContextOverridesBuilder = ImmutableMap.builder();
taskContextOverridesBuilder
.put(
MultiStageQueryContext.CTX_DURABLE_SHUFFLE_STORAGE,
isDurableStorageEnabled
).put(
MultiStageQueryContext.CTX_COMPOSED_INTERMEDIATE_SUPER_SORTER_STORAGE,
- MultiStageQueryContext.isComposedIntermediateSuperSorterStorageEnabled(
- task.getQuerySpec().getQuery().context()
- )
+ isDurableStorageEnabled // If Durable Storage is enabled, then super sorter intermediate storage can be enabled.
).put(
MultiStageQueryContext.CTX_INTERMEDIATE_SUPER_SORTER_STORAGE_MAX_LOCAL_BYTES,
Review Comment:
Lets update the documentation in `MultiStageQueryContext` for these params.
Also the user facing docs for durableStorage would need to be updated.
--
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
[GitHub] [druid] adarshsanjeev commented on a diff in pull request #14061: Move MSQ temporary storage to a runtime parameter instead of being configured from query context
Posted by "adarshsanjeev (via GitHub)" <gi...@apache.org>.
adarshsanjeev commented on code in PR #14061:
URL: https://github.com/apache/druid/pull/14061#discussion_r1169083375
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/error/NotEnoughTemporaryStorageFault.java:
##########
@@ -41,7 +41,8 @@ public NotEnoughTemporaryStorageFault(
{
super(
CODE,
- "Not enough temporary storage space. Requires at least %,d bytes. (configured = %,d bytes)",
+ "Not enough temporary storage space for intermediate files. Requires at least %,d bytes. (configured = %,d bytes). Increase the limit by increasing tmpStorageBytesPerTask or "
+ + "disable durable storage to not use disk for temporary intermediate files.",
Review Comment:
Changed
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerStorageParameters.java:
##########
@@ -58,11 +58,13 @@
*/
private static final long MINIMUM_SUPER_SORTER_TMP_STORAGE_BYTES = 1_000_000_000L;
+ private final boolean isIntermediateStorageLimitConfigured;
private final long intermediateSuperSorterStorageMaxLocalBytes;
- private WorkerStorageParameters(final long intermediateSuperSorterStorageMaxLocalBytes)
+ private WorkerStorageParameters(final long intermediateSuperSorterStorageMaxLocalBytes, final boolean isIntermediateStorageLimitConfigured)
Review Comment:
Updated
--
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
[GitHub] [druid] cryptoe commented on a diff in pull request #14061: Move MSQ temporary storage to a runtime parameter instead of being configured from query context
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #14061:
URL: https://github.com/apache/druid/pull/14061#discussion_r1168443597
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerStorageParameters.java:
##########
@@ -58,11 +58,13 @@
*/
private static final long MINIMUM_SUPER_SORTER_TMP_STORAGE_BYTES = 1_000_000_000L;
+ private final boolean isIntermediateStorageLimitConfigured;
private final long intermediateSuperSorterStorageMaxLocalBytes;
- private WorkerStorageParameters(final long intermediateSuperSorterStorageMaxLocalBytes)
+ private WorkerStorageParameters(final long intermediateSuperSorterStorageMaxLocalBytes, final boolean isIntermediateStorageLimitConfigured)
Review Comment:
Can we do this without the new class parameter `isIntermediateStorageLimitConfigured`.
if someone sets `intermediateSuperSorterStorageMaxLocalBytes` = -1 we treat the value as not set.
Meaning instead of the class parameter you would have a method which says
```
public boolean isIntermediateStorageLimitConfigured()
{
return !intermediateSuperSorterStorageMaxLocalBytes==-1;
}
```
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/error/NotEnoughTemporaryStorageFault.java:
##########
@@ -41,7 +41,8 @@ public NotEnoughTemporaryStorageFault(
{
super(
CODE,
- "Not enough temporary storage space. Requires at least %,d bytes. (configured = %,d bytes)",
+ "Not enough temporary storage space for intermediate files. Requires at least %,d bytes. (configured = %,d bytes). Increase the limit by increasing tmpStorageBytesPerTask or "
+ + "disable durable storage to not use disk for temporary intermediate files.",
Review Comment:
```suggestion
+ "disable durable storage by setting the context parameter blah blah blah.",
```
Something like this.
--
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
[GitHub] [druid] cryptoe merged pull request #14061: Move MSQ temporary storage to a runtime parameter instead of being configured from query context
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe merged PR #14061:
URL: https://github.com/apache/druid/pull/14061
--
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
[GitHub] [druid] adarshsanjeev commented on a diff in pull request #14061: Move MSQ temporary storage to a runtime parameter instead of being configured from query context
Posted by "adarshsanjeev (via GitHub)" <gi...@apache.org>.
adarshsanjeev commented on code in PR #14061:
URL: https://github.com/apache/druid/pull/14061#discussion_r1163669414
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/config/TaskConfig.java:
##########
@@ -78,6 +78,7 @@
private static final Period DEFAULT_DIRECTORY_LOCK_TIMEOUT = new Period("PT10M");
private static final Period DEFAULT_GRACEFUL_SHUTDOWN_TIMEOUT = new Period("PT5M");
private static final boolean DEFAULT_STORE_EMPTY_COLUMNS = true;
+ private static final long DEFAULT_TMP_STORAGE_BYTES_PER_TASK = Long.MAX_VALUE; // TODO: check!!!
Review Comment:
Added a documentation for this
--
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
[GitHub] [druid] adarshsanjeev commented on a diff in pull request #14061: Move MSQ temporary storage to a runtime parameter instead of being configured from query context
Posted by "adarshsanjeev (via GitHub)" <gi...@apache.org>.
adarshsanjeev commented on code in PR #14061:
URL: https://github.com/apache/druid/pull/14061#discussion_r1168178998
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/error/NotEnoughTemporaryStorageFault.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * 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.msq.indexing.error;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.annotation.JsonTypeName;
+
+import java.util.Objects;
+
+@JsonTypeName(NotEnoughTemporaryStorageFault.CODE)
+public class NotEnoughTemporaryStorageFault extends BaseMSQFault
+{
+ static final String CODE = "NotEnoughTemporaryStorageFault";
+
+ private final long suggestedMinimumStorage;
+ private final long configuredTemporaryStorage;
+
+ @JsonCreator
+ public NotEnoughTemporaryStorageFault(
+ @JsonProperty("suggestedMinimumStorage") final long suggestedMinimumStorage,
+ @JsonProperty("configuredTemporaryStorage") final long configuredTemporaryStorage
+ )
+ {
+ super(
+ CODE,
+ "Not enough temporary storage space. Requires at least %,d bytes. (configured = %,d bytes)",
Review Comment:
Updated
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerImpl.java:
##########
@@ -727,8 +740,8 @@ private OutputChannelFactory makeSuperSorterIntermediateOutputChannelFactory(
final FileOutputChannelFactory fileOutputChannelFactory =
new FileOutputChannelFactory(fileChannelDirectory, frameSize, intermediateSuperSorterLocalStorageTracker);
- if (MultiStageQueryContext.isComposedIntermediateSuperSorterStorageEnabled(QueryContext.of(task.getContext()))
Review Comment:
Changed to only trigger when the value is explicitly set in the run time parameters
--
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
[GitHub] [druid] cryptoe commented on pull request #14061: Move MSQ temporary storage to a runtime parameter instead of being configured from query context
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on PR #14061:
URL: https://github.com/apache/druid/pull/14061#issuecomment-1512908741
Merging since test coverage is complaining on classes where a constructor was changed.
--
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
[GitHub] [druid] cryptoe commented on pull request #14061: Move MSQ temporary storage to a runtime parameter instead of being configured from query context
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on PR #14061:
URL: https://github.com/apache/druid/pull/14061#issuecomment-1511675763
@adarshsanjeev Please add release notes for this PR .
--
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
[GitHub] [druid] adarshsanjeev commented on a diff in pull request #14061: Move MSQ temporary storage to a runtime parameter instead of being configured from query context
Posted by "adarshsanjeev (via GitHub)" <gi...@apache.org>.
adarshsanjeev commented on code in PR #14061:
URL: https://github.com/apache/druid/pull/14061#discussion_r1163666956
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java:
##########
@@ -588,21 +588,20 @@ private QueryDefinition initializeQueryDefAndState(final Closer closer)
.orElse(MSQWarnings.DEFAULT_MAX_PARSE_EXCEPTIONS_ALLOWED);
}
+ WorkerStorageParameters workerStorageParameters =
+ WorkerStorageParameters.createProductionInstanceForController(context.injector(), isDurableStorageEnabled);
+
ImmutableMap.Builder<String, Object> taskContextOverridesBuilder = ImmutableMap.builder();
taskContextOverridesBuilder
.put(
MultiStageQueryContext.CTX_DURABLE_SHUFFLE_STORAGE,
isDurableStorageEnabled
).put(
MultiStageQueryContext.CTX_COMPOSED_INTERMEDIATE_SUPER_SORTER_STORAGE,
Review Comment:
Removed, it should now read the runtime parameters in the worker directly
--
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
[GitHub] [druid] adarshsanjeev commented on a diff in pull request #14061: Move MSQ temporary storage to a runtime parameter instead of being configured from query context
Posted by "adarshsanjeev (via GitHub)" <gi...@apache.org>.
adarshsanjeev commented on code in PR #14061:
URL: https://github.com/apache/druid/pull/14061#discussion_r1163669040
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java:
##########
@@ -588,21 +588,20 @@ private QueryDefinition initializeQueryDefAndState(final Closer closer)
.orElse(MSQWarnings.DEFAULT_MAX_PARSE_EXCEPTIONS_ALLOWED);
}
+ WorkerStorageParameters workerStorageParameters =
+ WorkerStorageParameters.createProductionInstanceForController(context.injector(), isDurableStorageEnabled);
+
ImmutableMap.Builder<String, Object> taskContextOverridesBuilder = ImmutableMap.builder();
taskContextOverridesBuilder
.put(
MultiStageQueryContext.CTX_DURABLE_SHUFFLE_STORAGE,
isDurableStorageEnabled
).put(
MultiStageQueryContext.CTX_COMPOSED_INTERMEDIATE_SUPER_SORTER_STORAGE,
- MultiStageQueryContext.isComposedIntermediateSuperSorterStorageEnabled(
- task.getQuerySpec().getQuery().context()
- )
+ isDurableStorageEnabled // If Durable Storage is enabled, then super sorter intermediate storage can be enabled.
).put(
MultiStageQueryContext.CTX_INTERMEDIATE_SUPER_SORTER_STORAGE_MAX_LOCAL_BYTES,
Review Comment:
Updated the documentation
--
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
[GitHub] [druid] cryptoe commented on a diff in pull request #14061: Move MSQ temporary storage to a runtime parameter instead of being configured from query context
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #14061:
URL: https://github.com/apache/druid/pull/14061#discussion_r1166300286
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/error/NotEnoughTemporaryStorageFault.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * 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.msq.indexing.error;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.annotation.JsonTypeName;
+
+import java.util.Objects;
+
+@JsonTypeName(NotEnoughTemporaryStorageFault.CODE)
+public class NotEnoughTemporaryStorageFault extends BaseMSQFault
+{
+ static final String CODE = "NotEnoughTemporaryStorageFault";
+
+ private final long suggestedMinimumStorage;
+ private final long configuredTemporaryStorage;
+
+ @JsonCreator
+ public NotEnoughTemporaryStorageFault(
+ @JsonProperty("suggestedMinimumStorage") final long suggestedMinimumStorage,
+ @JsonProperty("configuredTemporaryStorage") final long configuredTemporaryStorage
+ )
+ {
+ super(
+ CODE,
+ "Not enough temporary storage space. Requires at least %,d bytes. (configured = %,d bytes)",
Review Comment:
We should also mention which runtime property to change here as well or disable durable storage using the context param.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerImpl.java:
##########
@@ -727,8 +740,8 @@ private OutputChannelFactory makeSuperSorterIntermediateOutputChannelFactory(
final FileOutputChannelFactory fileOutputChannelFactory =
new FileOutputChannelFactory(fileChannelDirectory, frameSize, intermediateSuperSorterLocalStorageTracker);
- if (MultiStageQueryContext.isComposedIntermediateSuperSorterStorageEnabled(QueryContext.of(task.getContext()))
Review Comment:
We should only use the composing output channel if the `tmpStorageBytesPerTask` is < Long.MAX value.
That way people who are using durable storage but have not configured `druid.indexer.task.tmpStorageBytesPerTask` go through the same code path.
--
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
[GitHub] [druid] adarshsanjeev commented on a diff in pull request #14061: Move MSQ temporary storage to a runtime parameter instead of being configured from query context
Posted by "adarshsanjeev (via GitHub)" <gi...@apache.org>.
adarshsanjeev commented on code in PR #14061:
URL: https://github.com/apache/druid/pull/14061#discussion_r1169010742
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerStorageParameters.java:
##########
@@ -58,11 +58,13 @@
*/
private static final long MINIMUM_SUPER_SORTER_TMP_STORAGE_BYTES = 1_000_000_000L;
+ private final boolean isIntermediateStorageLimitConfigured;
private final long intermediateSuperSorterStorageMaxLocalBytes;
- private WorkerStorageParameters(final long intermediateSuperSorterStorageMaxLocalBytes)
+ private WorkerStorageParameters(final long intermediateSuperSorterStorageMaxLocalBytes, final boolean isIntermediateStorageLimitConfigured)
Review Comment:
I initially did it this way since the ByteTracker still needs a default value(Long.MAX) if it is disabled. However, now I think it would be neater to move that check there instead to avoid the parameter.
--
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
[GitHub] [druid] adarshsanjeev commented on a diff in pull request #14061: Move MSQ temporary storage to a runtime parameter instead of being configured from query context
Posted by "adarshsanjeev (via GitHub)" <gi...@apache.org>.
adarshsanjeev commented on code in PR #14061:
URL: https://github.com/apache/druid/pull/14061#discussion_r1169711569
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/config/TaskConfig.java:
##########
@@ -137,7 +141,8 @@ public TaskConfig(
// deprecated, only set to true to fall back to older behavior
@JsonProperty("batchProcessingMode") String batchProcessingMode,
@JsonProperty("storeEmptyColumns") @Nullable Boolean storeEmptyColumns,
- @JsonProperty("encapsulatedTask") boolean enableTaskLevelLogPush
+ @JsonProperty("encapsulatedTask") boolean enableTaskLevelLogPush,
+ @JsonProperty("tmpStorageBytesPerTask") Long tmpStorageBytesPerTask
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@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
[GitHub] [druid] cryptoe commented on a diff in pull request #14061: Move MSQ temporary storage to a runtime parameter instead of being configured from query context
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #14061:
URL: https://github.com/apache/druid/pull/14061#discussion_r1169481531
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/config/TaskConfig.java:
##########
@@ -137,7 +141,8 @@ public TaskConfig(
// deprecated, only set to true to fall back to older behavior
@JsonProperty("batchProcessingMode") String batchProcessingMode,
@JsonProperty("storeEmptyColumns") @Nullable Boolean storeEmptyColumns,
- @JsonProperty("encapsulatedTask") boolean enableTaskLevelLogPush
+ @JsonProperty("encapsulatedTask") boolean enableTaskLevelLogPush,
+ @JsonProperty("tmpStorageBytesPerTask") Long tmpStorageBytesPerTask
Review Comment:
nit: should this be marked nullable ?
##########
docs/configuration/index.md:
##########
@@ -1521,6 +1521,7 @@ Additional peon configs include:
|`druid.indexer.task.restoreTasksOnRestart`|If true, MiddleManagers will attempt to stop tasks gracefully on shutdown and restore them on restart.|false|
|`druid.indexer.task.ignoreTimestampSpecForDruidInputSource`|If true, tasks using the [Druid input source](../ingestion/native-batch-input-source.md) will ignore the provided timestampSpec, and will use the `__time` column of the input datasource. This option is provided for compatibility with ingestion specs written before Druid 0.22.0.|false|
|`druid.indexer.task.storeEmptyColumns`|Boolean value for whether or not to store empty columns during ingestion. When set to true, Druid stores every column specified in the [`dimensionsSpec`](../ingestion/ingestion-spec.md#dimensionsspec). If you use schemaless ingestion and don't specify any dimensions to ingest, you must also set [`includeAllDimensions`](../ingestion/ingestion-spec.md#dimensionsspec) for Druid to store empty columns.<br/><br/>If you set `storeEmptyColumns` to false, Druid SQL queries referencing empty columns will fail. If you intend to leave `storeEmptyColumns` disabled, you should either ingest dummy data for empty columns or else not query on empty columns.<br/><br/>This configuration can be overwritten by setting `storeEmptyColumns` in the [task context](../ingestion/tasks.md#context-parameters).|true|
+|`druid.indexer.task.tmpStorageBytesPerTask`|Maximum number of bytes per task to be used to store temporary files on disk. This usage is split among all temporary storage usages for the task. An exception might be thrown if this limit is too low for the task or if this limit would be exceeded. This limit is currently respected only by MSQ tasks. Other types of tasks might exceed this limit. Must be greater than 0.|9223372036854775807|
Review Comment:
default limit is -1 rite ?
--
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
[GitHub] [druid] adarshsanjeev commented on a diff in pull request #14061: Move MSQ temporary storage to a runtime parameter instead of being configured from query context
Posted by "adarshsanjeev (via GitHub)" <gi...@apache.org>.
adarshsanjeev commented on code in PR #14061:
URL: https://github.com/apache/druid/pull/14061#discussion_r1169500286
##########
docs/configuration/index.md:
##########
@@ -1521,6 +1521,7 @@ Additional peon configs include:
|`druid.indexer.task.restoreTasksOnRestart`|If true, MiddleManagers will attempt to stop tasks gracefully on shutdown and restore them on restart.|false|
|`druid.indexer.task.ignoreTimestampSpecForDruidInputSource`|If true, tasks using the [Druid input source](../ingestion/native-batch-input-source.md) will ignore the provided timestampSpec, and will use the `__time` column of the input datasource. This option is provided for compatibility with ingestion specs written before Druid 0.22.0.|false|
|`druid.indexer.task.storeEmptyColumns`|Boolean value for whether or not to store empty columns during ingestion. When set to true, Druid stores every column specified in the [`dimensionsSpec`](../ingestion/ingestion-spec.md#dimensionsspec). If you use schemaless ingestion and don't specify any dimensions to ingest, you must also set [`includeAllDimensions`](../ingestion/ingestion-spec.md#dimensionsspec) for Druid to store empty columns.<br/><br/>If you set `storeEmptyColumns` to false, Druid SQL queries referencing empty columns will fail. If you intend to leave `storeEmptyColumns` disabled, you should either ingest dummy data for empty columns or else not query on empty columns.<br/><br/>This configuration can be overwritten by setting `storeEmptyColumns` in the [task context](../ingestion/tasks.md#context-parameters).|true|
+|`druid.indexer.task.tmpStorageBytesPerTask`|Maximum number of bytes per task to be used to store temporary files on disk. This usage is split among all temporary storage usages for the task. An exception might be thrown if this limit is too low for the task or if this limit would be exceeded. This limit is currently respected only by MSQ tasks. Other types of tasks might exceed this limit. Must be greater than 0.|9223372036854775807|
Review Comment:
Yes, missed updating this. Changed the documentation.
--
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