Skip to content

HBASE-26942 cache region locations when getAllRegionLocations #4335

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

Merged
bbeaudreault merged 9 commits into from
Apr 22, 2022
Merged

HBASE-26942 cache region locations when getAllRegionLocations #4335

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c73a751
HBASE-26942 cache region locations when getAllRegionLocations
Apr 10, 2022
329ef8b
replace future.whenComplete with FutureUtils.addListener
Apr 12, 2022
5c53f68
fix a re-complete future problem
Apr 12, 2022
3e1530e
add a test case for getting region locations exceptionally
Apr 13, 2022
9ccecce
fix the unrelated test case
Apr 13, 2022
98d106d
catch the exception and complete future exceptionnal while adding reg…
Apr 13, 2022
4f334a0
add a test case for getting region locations exceptionally
Apr 15, 2022
7da42da
fix a re-complete future mistake
Apr 16, 2022
c5798cd
remove the extra future
Apr 18, 2022
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
2 changes: 1 addition & 1 deletion hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncNonMetaRegionLocator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,7 @@ private void removeLocationFromCache(HRegionLocation loc) {
}
}

private void addLocationToCache(HRegionLocation loc) {
void addLocationToCache(HRegionLocation loc) {
addToCache(getTableCache(loc.getRegion().getTable()), createRegionLocations(loc));
}

Expand Down
5 changes: 4 additions & 1 deletion hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncTableRegionLocatorImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,10 @@ public CompletableFuture<List<HRegionLocation>> getAllRegionLocations() {
.thenApply(locs -> Arrays.asList(locs.getRegionLocations()));
}
return ClientMetaTableAccessor
.getTableHRegionLocations(conn.getTable(TableName.META_TABLE_NAME), tableName);
.getTableHRegionLocations(conn.getTable(TableName.META_TABLE_NAME), tableName)
.whenComplete((locs, error) -> {

@bbeaudreault bbeaudreault Apr 12, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You need to handle the case where error != null here. In that case I believe locs will be null, and you'll get an NPE below. Can you add a test for this?

Also, I think the convention in the async client is to use FutureUtils.addListener. The benefit there is it will also handle catching any errors thrown by your own callback.

@frostruan frostruan Apr 12, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your great suggestions.

From my understanding, if the error is not null, the get method on this future will get an ExecutionException, the region location caching code will not run, therefore there is no NPR problem here (Correct me if I am wrong). Based on this consideration I didn't handle the case error != null. But I agree that it's better to use FutureUtils.addListener. Thanks for reminding me about this. I'll address it as soon as possible.

Also, Congratulations on being HBase committer. @bbeaudreault

@bbeaudreault bbeaudreault Apr 12, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the congrats!

Here's the javadoc for whenComplete:

Returns a new CompletionStage with the same result or exception as this stage, that executes the given action when this stage completes.
When this stage is complete, the given action is invoked with the result (or null if none) and the exception (or null if none) of this stage as arguments. The returned stage is completed when the action returns.
Unlike method handle, this method is not designed to translate completion outcomes, so the supplied action should not throw an exception. However, if it does, the following rules apply: if this stage completed normally but the supplied action throws an exception, then the returned stage completes exceptionally with the supplied action's exception. Or, if this stage completed exceptionally and the supplied action throws an exception, then the returned stage completes exceptionally with this stage's exception.

My understanding of that is, if there were an error the callback would get called with locs == null and error != null. And then, your callback should not throw an exception. But since you're callback is called with locs == null, your callback is-was would throw an NPE. Then basd on the last 2 sentences, it would be the same result for the end-user, but they'd see an NPE instead of the originating error.

@bbeaudreault bbeaudreault Apr 12, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In terms of your recent change, I don't think it's necessary to re-complete the future within the addListener callback. I've been looking through other usages of the method and the only time they seem to do that is in cases where you're chaining together multiple async calls with one high level future. I could definitely be wrong about that though, and I'm not sure there's a harm in re-completing.

@frostruan frostruan Apr 12, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your patient reply. It helps me a lot.

After re-reading the doc and doing some tests, it turns out I was wrong before. Yes, there will indeed be NPE problem here. But based on the doc above and my tests, the user will still see the originating error. If the error is not null here, the future will completed exceptionally with this error no matter the supplied action throws an exception or not. Am I right?

@bbeaudreault bbeaudreault Apr 12, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No problem, thank you for verifying in tests. I think now that you are using addListener, you are correct. This is part of the benefit of addListener, but still better to avoid the exception altogether. I believe in your original implementation with your own withComplete call it would have resulted in an NPE to the user (but I could be wrong, it's been a while).

@bbeaudreault bbeaudreault Apr 12, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would you mind including a test case in TestAsyncNonMetaRegionLocator for the exceptional case?

@frostruan frostruan Apr 12, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK. I'll address it as soon as possible. Thanks.

locs.forEach(loc -> conn.getLocator().getNonMetaRegionLocator().addLocationToCache(loc));
});
}, getClass().getSimpleName() + ".getAllRegionLocations");
}

Expand Down
13 changes: 13 additions & 0 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncNonMetaRegionLocator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertSame;

import java.io.IOException;
Expand Down Expand Up @@ -460,4 +461,16 @@ public void testConcurrentUpdateCachedLocationOnError() throws Exception {
IntStream.range(0, 100).parallel()
.forEach(i -> locator.updateCachedLocationOnError(loc, new NotServingRegionException()));
}

@Test
public void testCacheLocationWhenGetAllLocations() throws Exception {
createMultiRegionTable();
AsyncConnectionImpl conn = (AsyncConnectionImpl)
ConnectionFactory.createAsyncConnection(TEST_UTIL.getConfiguration()).get();
conn.getRegionLocator(TABLE_NAME).getAllRegionLocations().get();
List<RegionInfo> regions = TEST_UTIL.getAdmin().getRegions(TABLE_NAME);
for (RegionInfo region : regions) {
assertNotNull(conn.getLocator().getRegionLocationInCache(TABLE_NAME, region.getStartKey()));
}
}
}