taklwu commented on code in PR #8851:
URL: https://github.com/apache/hbase/pull/8851#discussion_r4555347277
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileInfo.java:
##########
@@ -361,24 +400,58 @@ ReaderContext createReaderContext(boolean doDropBehind,
long readahead, ReaderTy
newFnfe.initCause(fnfe);
throw newFnfe;
}
- status = fs.getFileStatus(referencePath);
- } else {
- in = new FSDataInputStreamWrapper(fs, this.getPath(), doDropBehind,
readahead);
- status = fs.getFileStatus(initialPath);
- }
- long length = status.getLen();
- ReaderContextBuilder contextBuilder =
- new
ReaderContextBuilder().withInputStreamWrapper(in).withFileSize(length)
-
.withPrimaryReplicaReader(this.primaryReplica).withReaderType(type).withFileSystem(fs)
-
.withSystemKeyCache(systemKeyCache).withManagedKeyDataCache(managedKeyDataCache);
- if (this.reference != null) {
- contextBuilder.withFilePath(this.getPath());
+ FileStatus refStatus = fs.getFileStatus(referencePath);
+ readerPath = this.getPath();
+ length = refStatus.getLen();
} else {
- contextBuilder.withFilePath(status.getPath());
- }
+ statusForOpen = getPlainHFileStatusForOpen();
+ in = new FSDataInputStreamWrapper(fs, initialPath, doDropBehind,
readahead, statusForOpen);
+ readerPath = statusForOpen.getPath();
+ length = size > 0 ? size : statusForOpen.getLen();
+ }
+ ReaderContextBuilder contextBuilder = new
ReaderContextBuilder().withInputStreamWrapper(in)
+
.withFileSize(length).withPrimaryReplicaReader(this.primaryReplica).withReaderType(type)
+ .withFileSystem(fs).withSystemKeyCache(systemKeyCache)
+ .withManagedKeyDataCache(managedKeyDataCache).withFilePath(readerPath);
return contextBuilder.build();
}
+ private boolean isPlainHFile() {
+ return this.reference == null && this.link == null;
+ }
+
+ /**
+ * Returns a {@link FileStatus} for plain HFiles, using a cached or
synthesized status when
+ * possible so reader open does not repeat metadata lookups.
+ */
+ private FileStatus getPlainHFileStatusForOpen() throws IOException {
+ // A complete HFile is never empty, so a zero-length cached status was
captured before the
+ // file was fully written and must be refreshed.
+ if (plainHFileStatus != null && plainHFileStatus.getLen() > 0) {
Review Comment:
it should be a test issue, my understanding of the code in
`TestCachedMobFile.testOpenClose`, `StoreFileWriter.Builder.build()` creates
the HFile immediately, and the test calls `createStoreFileInfoForHFile()`
before `MobTestUtil.writeStoreFile()` writes any data.
That constructor stats the file (`fs.getFileStatus`), so the cached status
is a real filesystem status (`plainHFileStatusNeedsRefresh` = false), but it
was taken while the file was still empty, so `getLen()` == 0
```
FileStatus fStatus = fs.getFileStatus(initialPath);
this.createdTimestamp = fStatus.getModificationTime();
this.size = fStatus.getLen();
this.plainHFileStatus = fStatus;
this.plainHFileStatusNeedsRefresh = false;
```
cachedMobFile.open() leads to HStoreFile.initReader() and then
StoreFileInfo.createReaderContext().
- Before HBASE-30528: createReaderContext always called
fs.getFileStatus(initialPath) again, got the real length, and the test passed.
The re-stat hid the stale status.
- With HBASE-30528 but without the getLen() > 0 check: the cached status was
reused, the file size was size > 0 ? size : status.getLen(), which is 0, and
ReaderContextBuilder failed with Illegal ReaderContext, fileSize <= 0.
`plainHFileStatusNeedsRefresh` only marks statuses built in
`initPlainHFileStatusFromKnownSize()` (known size, no block info). Those are
fine for opening the reader but not for block locality, and they are never
length 0. The getLen() > 0 added here check covers the other case: a real
status that went stale because the file was written after it was captured. A
finished HFile always has a trailer, so length 0 reliably means "stat again".
I don't think if any production code will hit the same test flow that
creates a StoreFileInfo for an HFile before the file has been written and
closed. The order in TestCachedMobFile (create the StoreFileInfo, then write
the data) only happens in tests. so if we don't change the `TestCachedMobFile`
with writing data first before `createStoreFileInfoForHFile`, one direction I
can do is to add a `null` check with if getLen > 0, then we cache the
fileStatus. will that help ?
--
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]
