Skip to content

HBASE-23584 : Descrease rpc getFileStatus count when open a storefile #958

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
HuiHang-Yu wants to merge 8 commits into from
Closed

HBASE-23584 : Descrease rpc getFileStatus count when open a storefile #958

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a41081d
HBASE-23584 : Descrease rpc getFileStatus count when open a storefile
Dec 23, 2019
6800ae1
Update StoreFileInfo.java
HuiHang-Yu Dec 24, 2019
8cbca97
Update StoreFileInfo.java
HuiHang-Yu Dec 24, 2019
b122afd
Update StoreFileInfo.java
HuiHang-Yu Jan 2, 2020
cfa10c5
Update StoreFileInfo.java
HuiHang-Yu Jan 3, 2020
0948726
initialize localStatus on construction
HuiHang-Yu Mar 23, 2020
10cd97a
Merge pull request #1 from apache/master
HuiHang-Yu Mar 30, 2020
f1dac8f
Update StoreFileInfo.java
HuiHang-Yu Mar 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 19 additions & 10 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
public class StoreFileInfo {
private static final Logger LOG = LoggerFactory.getLogger(StoreFileInfo.class);

private FileStatus localStatus;
Copy link
Contributor

@saintstack saintstack Dec 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like idea of caching status. When does it get invalidated?

Copy link
Author

@HuiHang-Yu HuiHang-Yu Dec 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it will be updated when reopened only because hfile will not be modified after writen i think . Is there some scenarios fileStatus will be modified?

Copy link
Contributor

@saintstack saintstack Dec 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that I can think of.

Copy link
Contributor

@saintstack saintstack Dec 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it need to be volatile? Will StoreFileInfo be accessed by many threads? Can it be final/created on construction?

/**
* A non-capture group, for hfiles, so that this can be embedded.
* HFiles are uuid ([0-9a-z]+). Bulk loaded hfiles has (_SeqId_[0-9]+_) has suffix.
Expand Down Expand Up @@ -108,6 +109,7 @@ public class StoreFileInfo {
private RegionCoprocessorHost coprocessorHost;

// timestamp on when the file was created, is 0 and ignored for reference or link files
// the before timestamp is shown as above ! Now i change to use the createdTimestamp of reference or link files , will it create some problem later ?
Copy link
Contributor

@saintstack saintstack Dec 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is 'above'. Comments don't usually refer to 'i' since code is product of many. What is the problem this will create and if you don't know what it might be, why change i?

private long createdTimestamp;

private long size;
Expand Down Expand Up @@ -168,9 +170,8 @@ private StoreFileInfo(final Configuration conf, final FileSystem fs, final FileS
this.createdTimestamp = fileStatus.getModificationTime();
this.size = fileStatus.getLen();
} else {
FileStatus fStatus = fs.getFileStatus(initialPath);
this.createdTimestamp = fStatus.getModificationTime();
this.size = fStatus.getLen();
this.createdTimestamp = this.getFileStatus().getModificationTime();
Copy link
Contributor

@saintstack saintstack Dec 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, we do not put the 'this.' in front of local method invocation.

Copy link
Author

@HuiHang-Yu HuiHang-Yu Jan 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove it .

this.size = this.getFileStatus().getLen();
}
this.reference = null;
this.link = null;
Expand Down Expand Up @@ -296,7 +297,7 @@ ReaderContext createReaderContext(boolean doDropBehind, long readahead, ReaderTy
if (this.link != null) {
// HFileLink
in = new FSDataInputStreamWrapper(fs, this.link, doDropBehind, readahead);
status = this.link.getFileStatus(fs);
// status = this.link.getFileStatus(fs);
Copy link
Contributor

@saintstack saintstack Dec 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove rather than comment-out.

Copy link
Author

@HuiHang-Yu HuiHang-Yu Dec 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh my fault ! I will remove it later !

} else if (this.reference != null) {
// HFile Reference
Path referencePath = getReferredToFile(this.getPath());
Expand All @@ -310,11 +311,12 @@ ReaderContext createReaderContext(boolean doDropBehind, long readahead, ReaderTy
newFnfe.initCause(fnfe);
throw newFnfe;
}
status = fs.getFileStatus(referencePath);
// status = fs.getFileStatus(referencePath);
} else {
in = new FSDataInputStreamWrapper(fs, this.getPath(), doDropBehind, readahead);
status = fs.getFileStatus(initialPath);
// status = fs.getFileStatus(initialPath);
}
status = this.getFileStatus();
long length = status.getLen();
ReaderContextBuilder contextBuilder = new ReaderContextBuilder()
.withInputStreamWrapper(in)
Expand Down Expand Up @@ -370,13 +372,16 @@ private HDFSBlocksDistribution computeHDFSBlocksDistributionInternal(final FileS
*/
public FileStatus getReferencedFileStatus(final FileSystem fs) throws IOException {
FileStatus status;
if(this.localStatus !=null) return this.localStatus;
Copy link
Contributor

@saintstack saintstack Dec 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need parens as per convention in the code that surrounds this. Needs space after the '='.

Copy link
Author

@HuiHang-Yu HuiHang-Yu Dec 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

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);
this.localStatus = link.getFileStatus(fs);
status = this.localStatus;
return status;
} catch (FileNotFoundException ex) {
// try the other location
exToThrow = ex;
Expand All @@ -386,23 +391,27 @@ public FileStatus getReferencedFileStatus(final FileSystem fs) throws IOExceptio
} else {
// HFile Reference
Path referencePath = getReferredToFile(this.getPath());
status = fs.getFileStatus(referencePath);
this.localStatus = fs.getFileStatus(referencePath);
status = this.localStatus;
}
} else {
if (this.link != null) {
FileNotFoundException exToThrow = null;
for (int i = 0; i < this.link.getLocations().length; i++) {
// HFileLink
try {
return link.getFileStatus(fs);
this.localStatus = link.getFileStatus(fs);
status = this.localStatus;
return status;
Copy link
Contributor

@saintstack saintstack Dec 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is repeated. Put it into a method?

} catch (FileNotFoundException ex) {
// try the other location
exToThrow = ex;
}
}
throw exToThrow;
} else {
status = fs.getFileStatus(initialPath);
this.localStatus = fs.getFileStatus(initialPath);
status = this.localStatus;
Copy link
Contributor

@saintstack saintstack Dec 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three times.

Copy link
Author

@HuiHang-Yu HuiHang-Yu Dec 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea !

}
}
return status;
Expand Down