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:
[email protected]
