Copilot commented on code in PR #8851:
URL: https://github.com/apache/hbase/pull/8851#discussion_r4494223270
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java:
##########
@@ -217,6 +217,32 @@ public static FSDataOutputStream create(Configuration
conf, FileSystem fs, Path
return create(conf, fs, path, perm, favoredNodes, true);
}
+ /**
+ * Opens a file for reading. When {@code fileStatus} is supplied and the
{@link FileSystem}
+ * supports {@code FileSystem.openFile().withFileStatus()}, that path is
used so the client
+ * can skip a redundant metadata lookup (for example {@code getFileInfo}).
+ * @param fs filesystem
+ * @param path file to open
+ * @param fileStatus status from a recent {@code listStatus} / {@code
getFileStatus}, or
+ * {@code null}
+ * @return input stream for the file
+ * @throws IOException if the file cannot be opened
+ */
+ public static FSDataInputStream openForRead(FileSystem fs, Path path,
FileStatus fileStatus)
+ throws IOException {
+ if (fileStatus != null) {
+ try {
+ return fs.openFile(path).withFileStatus(fileStatus).build().get();
Review Comment:
The new tests count `getFileStatus` only on a `FilterFileSystem`; metadata
lookups inside the backing filesystem's open implementation bypass that
counter. Consequently, replacing this entire optimization with `fs.open(path)`
would still satisfy the reader-open assertions. Please add focused
`TestFSUtils` tests that verify the builder receives the supplied status and
legacy open is not used on success, plus a test that an unsupported builder
falls back to legacy open.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerBase.java:
##########
@@ -340,7 +342,7 @@ public StoreFileInfo getStoreFileInfo(FileStatus
fileStatus, Path initialPath,
throw new IOException("path=" + p + " doesn't look like a valid
StoreFile");
}
return new StoreFileInfo(conf, fs, createdTimestamp, initialPath, size,
reference, link,
- isPrimaryReplica);
+ isPrimaryReplica, listedPlainHFileStatus);
Review Comment:
The path-only overload already obtains a full `fStatus` at lines 337–339,
but passes null here, so `StoreFileInfo` synthesizes a status marked as needing
refresh. Since `HStoreFile.open()` calls `initHDFSBlocksDistribution()` before
creating the reader, this forces another stat of the same file despite already
having its full status. Preserve `fStatus` in `listedPlainHFileStatus` as well,
and update the no-status tracker test to expect no subsequent refresh.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java:
##########
@@ -217,6 +217,32 @@ public static FSDataOutputStream create(Configuration
conf, FileSystem fs, Path
return create(conf, fs, path, perm, favoredNodes, true);
}
+ /**
+ * Opens a file for reading. When {@code fileStatus} is supplied and the
{@link FileSystem}
+ * supports {@code FileSystem.openFile().withFileStatus()}, that path is
used so the client
+ * can skip a redundant metadata lookup (for example {@code getFileInfo}).
+ * @param fs filesystem
+ * @param path file to open
+ * @param fileStatus status from a recent {@code listStatus} / {@code
getFileStatus}, or
+ * {@code null}
+ * @return input stream for the file
+ * @throws IOException if the file cannot be opened
+ */
+ public static FSDataInputStream openForRead(FileSystem fs, Path path,
FileStatus fileStatus)
+ throws IOException {
+ if (fileStatus != null) {
+ try {
+ return fs.openFile(path).withFileStatus(fileStatus).build().get();
+ } catch (UnsupportedOperationException | InterruptedException |
ExecutionException e) {
Review Comment:
An interrupted wait is treated as an unsupported open and falls through to
`fs.open(path)`. `Future.get()` clears the interrupt flag when it throws
`InterruptedException`, so cancellation is lost and the thread starts another
filesystem operation instead of stopping. Please catch interruption separately,
restore the interrupt flag, and propagate an `InterruptedIOException`.
--
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]
