hanishakoneru commented on a change in pull request #2203:
URL: https://github.com/apache/ozone/pull/2203#discussion_r632058083
##########
File path:
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java
##########
@@ -507,4 +457,68 @@ private void handleReadError(IOException cause) throws
IOException {
public synchronized List<ChunkInputStream> getChunkStreams() {
return chunkStreams;
}
+
+ @Override
+ public synchronized int read(ByteBuffer byteBuffer) throws IOException {
Review comment:
Can we move this and readWithStrategy() methods up and place it after
the read(byte[], int, int) method. It would help improve code readability if we
club similar methods together.
##########
File path:
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/read/TestKeyInputStream.java
##########
@@ -200,6 +201,8 @@ public void testReadChunk() throws Exception {
for (int bufferSize : bufferSizeList) {
assertReadFully(data, keyInputStream, bufferSize, 0);
keyInputStream.seek(0);
+ assertReadFullyUseByteBuffer(data, keyInputStream, bufferSize, 0);
+ keyInputStream.seek(0);
Review comment:
Can we create a new test for ByteBuffer read?
##########
File path:
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ChunkInputStream.java
##########
@@ -191,6 +192,41 @@ public synchronized int read(byte[] b, int off, int len)
throws IOException {
return total;
}
+ @Override
+ public synchronized int read(ByteBuffer byteBuffer) throws IOException {
+ if (byteBuffer == null) {
+ throw new NullPointerException();
+ }
+ int bufferLen = byteBuffer.remaining();
Review comment:
Can we rename "bufferLen" to something to specify it is the remaining
number of bytes in the buffer (like "bufferLenRemaining" or just "len")
##########
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/client/io/LengthInputStream.java
##########
@@ -46,4 +51,14 @@ public long getLength() {
public InputStream getWrappedStream() {
return in;
}
+
+ @Override
+ public int read(ByteBuffer byteBuffer) throws IOException {
+ if (in instanceof ByteBufferReadable) {
+ return ((ByteBufferReadable)in).read(byteBuffer);
+ } else {
+ throw new UnsupportedOperationException("Read with ByteBuffer is not " +
+ " supported by this input stream");
Review comment:
Instead of "this" can we specify the class of input stream?
##########
File path:
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ByteArrayStrategy.java
##########
@@ -0,0 +1,60 @@
+/*
+ * 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.hadoop.hdds.scm.storage;
+
+import com.google.common.base.Preconditions;
+import java.io.IOException;
+import java.io.InputStream;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
+public class ByteArrayStrategy implements ReaderStrategy {
Review comment:
Can you please add a Javadoc for this class
##########
File path:
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/read/TestKeyInputStream.java
##########
@@ -351,4 +362,25 @@ private void assertReadFully(byte[] data, InputStream in,
}
Assert.assertEquals(data.length, totalRead);
}
+
+ private void assertReadFullyUseByteBuffer(byte[] data, KeyInputStream in,
Review comment:
NIT: assertReadFullyUsingByteBuffer
##########
File path:
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java
##########
@@ -507,4 +457,68 @@ private void handleReadError(IOException cause) throws
IOException {
public synchronized List<ChunkInputStream> getChunkStreams() {
return chunkStreams;
}
+
+ @Override
+ public synchronized int read(ByteBuffer byteBuffer) throws IOException {
+ ReaderStrategy strategy = new ByteBufferStrategy(byteBuffer);
+ int bufferLen = strategy.getTargetLength();
+ if (bufferLen == 0) {
+ return 0;
+ }
+ return readWithStrategy(strategy);
+ }
+
+ synchronized int readWithStrategy(ReaderStrategy strategy) throws
+ IOException {
+ Preconditions.checkArgument(strategy != null);
+ if (!initialized) {
+ initialize();
+ }
+
+ checkOpen();
+ int totalReadLen = 0;
+ int bufferLen = strategy.getTargetLength();
Review comment:
`bufferLen` here represents the number of bytes remaining to be read
into byteArray/ byteBuffer. Can we keep the variable name as `len` to avoid
confusion with total length of buffer.
##########
File path:
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/OzoneInputStream.java
##########
@@ -52,6 +55,16 @@ public int read(byte[] b, int off, int len) throws
IOException {
return inputStream.read(b, off, len);
}
+ @Override
+ public int read(ByteBuffer byteBuffer) throws IOException {
+ if (inputStream instanceof ByteBufferReadable) {
+ return ((ByteBufferReadable)inputStream).read(byteBuffer);
+ } else {
+ throw new UnsupportedOperationException("Read with ByteBuffer is not " +
+ " supported by this detail input stream");
Review comment:
Instead of "this" can we specify the class of input stream?
##########
File path:
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ByteArrayStrategy.java
##########
@@ -0,0 +1,60 @@
+/*
+ * 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.hadoop.hdds.scm.storage;
+
+import com.google.common.base.Preconditions;
+import java.io.IOException;
+import java.io.InputStream;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
+public class ByteArrayStrategy implements ReaderStrategy {
Review comment:
Can we rename this class to something to specify that it is a Reader
(for example ByteArrayReader like other Readers - FileReader etc).
##########
File path:
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ByteBufferStrategy.java
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.hadoop.hdds.scm.storage;
+
+import com.google.common.base.Preconditions;
+import org.apache.hadoop.fs.ByteBufferReadable;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.ByteBuffer;
+
+public class ByteBufferStrategy implements ReaderStrategy {
Review comment:
Same as for ByteArrayStrategy - It would be good if classname denotes
its a Reader class, and class needs Javadoc.
##########
File path:
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ReaderStrategy.java
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.hadoop.hdds.scm.storage;
+
+import java.io.IOException;
+import java.io.InputStream;
+
+public interface ReaderStrategy {
Review comment:
Javadoc missing.
##########
File path:
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyInputStream.java
##########
@@ -216,18 +222,32 @@ public synchronized int read() throws IOException {
*/
@Override
public synchronized int read(byte[] b, int off, int len) throws IOException {
- checkOpen();
- if (b == null) {
- throw new NullPointerException();
- }
- if (off < 0 || len < 0 || len > b.length - off) {
- throw new IndexOutOfBoundsException();
+ ReaderStrategy strategy = new ByteArrayStrategy(b, off, len);
+ int bufferLen = strategy.getTargetLength();
+ if (bufferLen == 0) {
+ return 0;
}
- if (len == 0) {
+ return readWithStrategy(strategy);
+ }
+
+ @Override
+ public synchronized int read(ByteBuffer byteBuffer) throws IOException {
+ ReaderStrategy strategy = new ByteBufferStrategy(byteBuffer);
+ int bufferLen = strategy.getTargetLength();
Review comment:
Same comment for `bufferLen`. It can be confused with length of the
buffer instead of remaining bytes in the buffer/ array.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
