gianm commented on code in PR #16790: URL: https://github.com/apache/druid/pull/16790#discussion_r1696274781
########## extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/shuffle/output/StageOutputReader.java: ########## @@ -0,0 +1,55 @@ +/* + * 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.shuffle.output; + +import com.google.common.util.concurrent.ListenableFuture; +import org.apache.druid.frame.channel.ReadableFrameChannel; +import org.apache.druid.msq.kernel.StageId; +import org.apache.druid.msq.shuffle.input.WorkerOrLocalInputChannelFactory; + +import java.io.Closeable; +import java.io.InputStream; + +/** + * Interface for remotely reading output channels for a particular stage. Each instance of this interface represents a + * stream from a single {@link org.apache.druid.msq.kernel.StagePartition} in + * {@link org.apache.druid.frame.file.FrameFile} format. + */ +public interface StageOutputReader extends Closeable +{ + /** + * Returns an {@link InputStream} starting from a particular point in the + * {@link org.apache.druid.frame.file.FrameFile}. Length of the stream is implementation-dependent; it may or may + * not go all the way to the end of the file. Zero-length stream indicates EOF. Any nonzero length means you should + * call this method again with a higher offset. + * + * @param offset offset into the frame file + * + * @see org.apache.druid.msq.exec.WorkerImpl#readChannel(StageId, int, long) + */ + ListenableFuture<InputStream> readRemotelyFrom(long offset); Review Comment: > Should it be ReadableFrameChannel instead for having a unified interface that the callers can call? The methods have different returns because they are meant for different uses: the first is for reading remotely (so it must return some bytes that can be sent over HTTP) and the second is for reading locally (so it should return a channel object). For this reason the names also make sense (to me 😄). If you have suggestions for better names LMK, otherwise I can keep them as-is. I've added additional javadocs to `StageOutputReader` and also its main implementations (`ChannelStageOutputReader` and `FileStageOutputReader`) to clarify the design. Please LMK if they do help in understanding. > the Javadoc should state what will happen if two calls are made - at t1: readRemotelyFrom(offset1) and at t2: readRemotelyFrom(offset2) when t1 < t2 and offset1 > offset2. From the implementation of the channel reader, it would throw an error. I have added this: ``` * It is implementation-dependent whether calls to this method must have monotonically increasing offsets. * In particular, {@link ChannelStageOutputReader} requires monotonically increasing offsets, but other * implementations do not. ``` I've also added details to the javadocs for `ChannelStageOutputReader` and `FileStageOutputReader`. -- 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]
