saintstack commented on a change in pull request #3596:
URL: https://github.com/apache/hbase/pull/3596#discussion_r703630434
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileInfo.java
##########
@@ -303,17 +302,16 @@ ReaderContext createReaderContext(boolean doDropBehind,
long readahead, ReaderTy
try {
in = new FSDataInputStreamWrapper(fs, referencePath, doDropBehind,
readahead);
} catch (FileNotFoundException fnfe) {
- // Intercept the exception so can insert more info about the
Reference; otherwise
- // exception just complains about some random file -- operator doesn't
realize it
- // other end of a Reference
- FileNotFoundException newFnfe = new FileNotFoundException(toString());
- newFnfe.initCause(fnfe);
- throw newFnfe;
+ throw decorateFileNotFoundException(fnfe);
}
status = fs.getFileStatus(referencePath);
} else {
in = new FSDataInputStreamWrapper(fs, this.getPath(), doDropBehind,
readahead);
- status = fs.getFileStatus(initialPath);
+ if (this.cachedFileStatus == null) {
+ // Safe to cache filestatus for a plain storefile/hfile.
+ this.cachedFileStatus = this.fs.getFileStatus(this.initialPath);
Review comment:
Yeah.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileInfo.java
##########
@@ -333,76 +331,76 @@ ReaderContext createReaderContext(boolean doDropBehind,
long readahead, ReaderTy
/**
* Compute the HDFS Block Distribution for this StoreFile
*/
- public HDFSBlocksDistribution computeHDFSBlocksDistribution(final FileSystem
fs)
- throws IOException {
- // guard against the case where we get the FileStatus from link, but by
the time we
- // call compute the file is moved again
+ public HDFSBlocksDistribution computeHDFSBlocksDistribution() throws
IOException {
if (this.link != null) {
+ // Guard against case where file behind link has moved when we go to
calculate distribution;
+ // e.g. from data dir to archive dir. Retry up to number of locations
under the link.
FileNotFoundException exToThrow = null;
for (int i = 0; i < this.link.getLocations().length; i++) {
try {
- return computeHDFSBlocksDistributionInternal(fs);
- } catch (FileNotFoundException ex) {
- // try the other location
- exToThrow = ex;
+ return computeHDFSBlocksDistributionInternal();
+ } catch (FileNotFoundException fnfe) {
+ // Try the other locations -- file behind link may have moved.
+ exToThrow = fnfe;
}
}
- throw exToThrow;
+ throw decorateFileNotFoundException(exToThrow);
} else {
- return computeHDFSBlocksDistributionInternal(fs);
+ return computeHDFSBlocksDistributionInternal();
}
}
- private HDFSBlocksDistribution computeHDFSBlocksDistributionInternal(final
FileSystem fs)
- throws IOException {
- FileStatus status = getReferencedFileStatus(fs);
+ private HDFSBlocksDistribution computeHDFSBlocksDistributionInternal()
throws IOException {
+ FileStatus status = getFileStatus();
if (this.reference != null) {
- return computeRefFileHDFSBlockDistribution(fs, reference, status);
+ return computeRefFileHDFSBlockDistribution(status);
} else {
- return FSUtils.computeHDFSBlocksDistribution(fs, status, 0,
status.getLen());
+ return FSUtils.computeHDFSBlocksDistribution(this.fs, status, 0,
status.getLen());
}
}
/**
- * Get the {@link FileStatus} of the file referenced by this StoreFileInfo
- * @param fs The current file system to use.
+ * Get the {@link FileStatus} of the file referenced by this StoreFileInfo
on the passed
+ * <code>fs</code>.
+ * This {@link StoreFileInfo} could be for a link or a reference or a plain
hfile/storefile; get
+ * the filestatus for whatever the link or reference points to (or just the
plain hfile/storefile
+ * if not a link/reference). This method is called by snapshot. Filesystem
may not be same as
+ * that of this hosting {@link StoreFileInfo}. If a link, when you go to use
the passed
+ * FileStatus, the file may have moved; e.g. from data to archive... be
aware.
+ * @param fs The filesystem to use.
* @return The {@link FileStatus} of the file referenced by this
StoreFileInfo
*/
public FileStatus getReferencedFileStatus(final FileSystem fs) throws
IOException {
+ if (this.fs.equals(fs) && this.cachedFileStatus != null) {
+ return this.cachedFileStatus;
+ }
FileStatus status;
if (this.reference != null) {
if (this.link != null) {
- FileNotFoundException exToThrow = null;
- for (int i = 0; i < this.link.getLocations().length; i++) {
- // HFileLink Reference
- try {
- return link.getFileStatus(fs);
- } catch (FileNotFoundException ex) {
- // try the other location
- exToThrow = ex;
- }
- }
- throw exToThrow;
+ status = this.link.getFileStatus(this.fs);
Review comment:
To be safe, it should use the passed param. This method is used outside
of this class. I looked at shutting it down but that would be bigger change.
Let me update. Thanks @anoopsjohn
--
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]
