[GitHub] [hbase] huaxiangsun commented on a change in pull request #1903: HBASE-24562: Stabilize master startup with meta replicas enabled

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

huaxiangsun commented on a change in pull request #1903:
URL: https://github.com/apache/hbase/pull/1903#discussion_r441783237



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
##########
@@ -1166,7 +1166,11 @@ private void finishActiveMasterInitialization(MonitoredTask status) throws IOExc
     assignmentManager.checkIfShouldMoveSystemRegionAsync();
     status.setStatus("Assign meta replicas");
     MasterMetaBootstrap metaBootstrap = createMetaBootstrap();
-    metaBootstrap.assignMetaReplicas();
+    try {
+      metaBootstrap.assignMetaReplicas();
+    } catch (HBaseIOException e){

Review comment:
       assignMetaReplicas() throws IOException, InterruptedException, and KeeperException. Why only HBaseIOException is processed here? Should it catch all exceptions and totally ignore all exceptions caused by assignMetaReplicas()?

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java
##########
@@ -616,6 +616,24 @@ public long assign(RegionInfo regionInfo) throws IOException {
     return assign(regionInfo, null);
   }
 
+  public Future<byte[]> assignAsync(RegionInfo regionInfo, ServerName sn) throws IOException {

Review comment:
       Most of this method's code is a duplicate  of the assign() above, the common part can be wrapped in a private method.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java
##########
@@ -616,6 +616,24 @@ public long assign(RegionInfo regionInfo) throws IOException {
     return assign(regionInfo, null);
   }
 
+  public Future<byte[]> assignAsync(RegionInfo regionInfo, ServerName sn) throws IOException {

Review comment:
       If we are adding a new public method, can we have doc for these two new methods?

##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestMetaWithReplicas.java
##########
@@ -355,4 +362,58 @@ public void testShutdownOfReplicaHolder() throws Exception {
       assertNotEquals(3, i);
     }
   }
+
+  @Test
+  public void testFailedReplicaAssigment() throws InterruptedException, IOException {

Review comment:
       Have one question about the test case, do not see any exceptions thrown out during assigning meta replica regions, how do we make sure the following code is working.
   ```
       try {
         metaBootstrap.assignMetaReplicas();
       } catch (HBaseIOException e){
   ```
     If we can make sure that the test can compile with or without the changes in non-test modules, it will be great as we can verify that without those changes, the test will fail. Seems with current implementation, it is not possible.




----------------------------------------------------------------
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.

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