steveloughran commented on code in PR #4414:
URL: https://github.com/apache/hbase/pull/4414#discussion_r873936144
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/util/BlockIOUtils.java:
##########
@@ -228,21 +228,43 @@ public static boolean readWithExtra(ByteBuff buf,
FSDataInputStream dis, int nec
*/
public static boolean preadWithExtra(ByteBuff buff, FSDataInputStream dis,
long position,
int necessaryLen, int extraLen) throws IOException {
+ return preadWithExtra(buff, dis, position, necessaryLen, extraLen, false);
+ }
+
+ /**
+ * Read from an input stream at least <code>necessaryLen</code> and if
possible,
+ * <code>extraLen</code> also if available. Analogous to
+ * {@link IOUtils#readFully(InputStream, byte[], int, int)}, but uses
positional read and
+ * specifies a number of "extra" bytes that would be desirable but not
absolutely necessary to
+ * read. If the input stream supports ByteBufferPositionedReadable, it reads
to the byte buffer
+ * directly, and does not allocate a temporary byte array.
+ * @param buff ByteBuff to read into.
+ * @param dis the input stream to read from
+ * @param position the position within the stream from which to start
reading
+ * @param necessaryLen the number of bytes that are absolutely necessary to
read
+ * @param extraLen the number of extra bytes that would be nice to read
+ * @param readAllBytes whether we must read the necessaryLen and extraLen
+ * @return true if and only if extraLen is > 0 and reading those extra bytes
was successful
+ * @throws IOException if failed to read the necessary bytes
+ */
+ public static boolean preadWithExtra(ByteBuff buff, FSDataInputStream dis,
long position,
+ int necessaryLen, int extraLen, boolean readAllBytes) throws IOException {
boolean preadbytebuffer = dis.hasCapability("in:preadbytebuffer");
if (preadbytebuffer) {
- return preadWithExtraDirectly(buff, dis, position, necessaryLen,
extraLen);
+ return preadWithExtraDirectly(buff, dis, position, necessaryLen,
extraLen, readAllBytes);
} else {
- return preadWithExtraOnHeap(buff, dis, position, necessaryLen, extraLen);
+ return preadWithExtraOnHeap(buff, dis, position, necessaryLen, extraLen,
readAllBytes);
}
}
private static boolean preadWithExtraOnHeap(ByteBuff buff, FSDataInputStream
dis, long position,
- int necessaryLen, int extraLen) throws IOException {
+ int necessaryLen, int extraLen, boolean readAllBytes) throws IOException {
int remain = necessaryLen + extraLen;
byte[] buf = new byte[remain];
int bytesRead = 0;
- while (bytesRead < necessaryLen) {
+ int lengthMustRead = readAllBytes ? remain : necessaryLen;
+ while (bytesRead < lengthMustRead) {
int ret = dis.read(position + bytesRead, buf, bytesRead, remain);
Review Comment:
any reason for not using dis.readFully() into the byte array?
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/util/BlockIOUtils.java:
##########
@@ -257,11 +279,12 @@ private static boolean preadWithExtraOnHeap(ByteBuff
buff, FSDataInputStream dis
}
private static boolean preadWithExtraDirectly(ByteBuff buff,
FSDataInputStream dis, long position,
Review Comment:
in line 305 on this method, the NPE raised should be included as the cause
of the IOE. that way if the input stream code raises NPEs, debugging it becomes
possible
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestBlockIOUtils.java:
##########
@@ -92,6 +109,103 @@ public void testReadFully() throws IOException {
assertArrayEquals(Bytes.toBytes(s), heapBuf);
}
+ @Test
+ public void testPreadWithReadFullBytes() throws IOException {
+ testPreadReadFullBytesInternal(true, EnvironmentEdgeManager.currentTime());
+ }
+
+ @Test
+ public void testPreadWithoutReadFullBytes() throws IOException {
+ testPreadReadFullBytesInternal(false,
EnvironmentEdgeManager.currentTime());
+ }
+
+ private void testPreadReadFullBytesInternal(boolean readAllBytes, long
randomSeed)
+ throws IOException {
+ Configuration conf = TEST_UTIL.getConfiguration();
+ conf.setBoolean(HConstants.HFILE_PREAD_ALL_BYTES_ENABLED_KEY,
readAllBytes);
+ FileSystem fs = TEST_UTIL.getTestFileSystem();
+ Path path = new Path(TEST_UTIL.getDataTestDirOnTestFS(),
testName.getMethodName());
+ // give a fixed seed such we can see failure easily.
+ Random rand = new Random(randomSeed);
+ long totalDataBlockBytes =
+ writeBlocks(TEST_UTIL.getConfiguration(), rand, COMPRESSION_ALGO, path);
+ readDataBlocksAndVerify(fs, path, COMPRESSION_ALGO, totalDataBlockBytes);
+ }
+
+ private long writeBlocks(Configuration conf, Random rand,
Compression.Algorithm compressAlgo,
+ Path path) throws IOException {
+ FileSystem fs = HFileSystem.get(conf);
+ FSDataOutputStream os = fs.create(path);
Review Comment:
safer within try/finally or try with resources
--
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: issues-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
