LakshSingla commented on code in PR #13368:
URL: https://github.com/apache/druid/pull/13368#discussion_r1022715086
##########
core/src/test/java/org/apache/druid/storage/local/LocalFileStorageConnectorTest.java:
##########
@@ -151,6 +152,33 @@ public void listFilesTest() throws Exception
);
}
+ @Test
+ public void testReadRange() throws Exception
Review Comment:
Thanks for the clear and comprehensive test case!
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java:
##########
@@ -1187,7 +1187,7 @@ private Yielder<Object[]> getFinalResultsYielder(
final InputChannelFactory inputChannelFactory;
- if
(MultiStageQueryContext.isDurableStorageEnabled(task.getQuerySpec().getQuery().context()))
{
+ if
(MultiStageQueryContext.isDurableShuffleStorageEnabled(task.getQuerySpec().getQuery().context()))
{
Review Comment:
nit: While the changes are not made in this PR, I think we can reuse
`isDurableStorageEnabled` instead of this method call again.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/util/MultiStageQueryContext.java:
##########
@@ -60,6 +61,9 @@
public static final String CTX_ENABLE_DURABLE_SHUFFLE_STORAGE =
"durableShuffleStorage";
private static final boolean DEFAULT_ENABLE_DURABLE_SHUFFLE_STORAGE = false;
+ public static final String CTX_ENABLE_DURABLE_TASK_INTERMEDIATE_STORAGE =
"durableTaskIntermediateStorage";
Review Comment:
I think that we should somehow try to merge this config property with
`durableShuffleStorage` to make it easier for the users to configure. Adding
another property means that the user can now have up to 4 configurations of
`durableShuffleStorage` and `durableTaskIntermediateStorage` each with
different semantics. WDYT?
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerImpl.java:
##########
@@ -183,7 +184,10 @@ public WorkerImpl(MSQWorkerTask task, WorkerContext
context)
this.context = context;
this.selfDruidNode = context.selfNode();
this.processorBouncer = context.processorBouncer();
- this.durableStageStorageEnabled =
MultiStageQueryContext.isDurableStorageEnabled(
+ this.durableStageStorageEnabled =
MultiStageQueryContext.isDurableShuffleStorageEnabled(
+ QueryContext.of(task.getContext())
+ );
+ this.durableTaskIntermediateStorageEnabled =
MultiStageQueryContext.isDurableTaskIntermediateStorageEnabled(
Review Comment:
I think for this parameter to get populated from the user query, we would
need to pass it explicitly in the `MSQWorkerTaskLauncher` class. There should
be a preexisting example of `CTX_ENABLE_DURABLE_SHUFFLE_STORAGE` in there
already.
##########
core/src/main/java/org/apache/druid/storage/StorageConnector.java:
##########
@@ -75,6 +75,19 @@
*/
InputStream read(String path) throws IOException;
+ /**
+ * Reads the data present for a given range at the path in the underlying
storage system.
+ * Most implementations prepend the input path with a basePath.
+ * The caller should take care of closing the stream when done or in case of
error. Further, the caller must ensure
+ * that the start offset and the size of the read are valid parameters for
the given path for correct behavior.
+ * @param path The path to read data from
+ * @param from Start offset of the read in the path
+ * @param size Length of the read to be done
+ * @return InputStream starting from the given offset limited by the given
size
+ * @throws IOException if the path is not present or the unable to read the
data present on the path
+ */
+ InputStream readRange(String path, long from, long size) throws IOException;
Review Comment:
Should the behaviour of the implementations be defined here under following
conditions:
1. from > size of underlying storage (undefined, error, or we return empty
output)
2. from + size > size of underlying storage (undefined, error, or we chomp
the stream)
3. from < 0 (undefined, error, or from indicates index from the end)
4. size < 0 (undefined, error, or we read in reverse)
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java:
##########
@@ -522,7 +522,7 @@ private QueryDefinition initializeQueryDefAndState(final
Closer closer)
closer.register(netClient::close);
final boolean isDurableStorageEnabled =
Review Comment:
```suggestion
final boolean isDurableShufleStorageEnabled =
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
