[GitHub] [hbase] frostruan commented on a diff in pull request #4335: HBASE-26942 cache region locations when getAllRegionLocations

[GitBox <gi...@apache.org>]

frostruan commented on code in PR #4335:
URL: https://github.com/apache/hbase/pull/4335#discussion_r848582891


##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncTableRegionLocatorImpl.java:
##########
@@ -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) -> {

Review Comment:
   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 



-- 
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: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org