[GitHub] [hbase] busbey commented on a change in pull request #2172: HBASE-24795 : RegionMover to deal with unknown region while (un)loading

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

busbey commented on a change in pull request #2172:
URL: https://github.com/apache/hbase/pull/2172#discussion_r463138413



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/util/MoveWithoutAck.java
##########
@@ -0,0 +1,70 @@
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hbase.util;
+
+import org.apache.hadoop.hbase.ServerName;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.List;
+import java.util.concurrent.Callable;
+
+/**
+ * Move Regions without Acknowledging.Usefule in case of RS shutdown as we might want to shut the
+ * RS down anyways and not abort on a stuck region. Improves movement performance
+ */
+@InterfaceAudience.Private
+class MoveWithoutAck implements Callable<Boolean> {
+
+  private static final Logger LOG = LoggerFactory.getLogger(MoveWithoutAck.class);
+
+  private final RegionInfo region;
+  private final ServerName targetServer;
+  private final List<RegionInfo> movedRegions;
+  private final ServerName sourceServer;
+  private final Admin admin;
+
+  MoveWithoutAck(Admin admin, RegionInfo regionInfo, ServerName sourceServer,
+    ServerName targetServer, List<RegionInfo> movedRegions) {
+    this.admin = admin;
+    this.region = regionInfo;
+    this.targetServer = targetServer;
+    this.movedRegions = movedRegions;
+    this.sourceServer = sourceServer;
+  }
+
+  @Override
+  public Boolean call() {
+    try {
+      LOG.info("Moving region: {} from {} to {}", region.getEncodedName(), sourceServer,
+        targetServer);
+      admin.move(region.getEncodedNameAsBytes(), targetServer);
+      LOG.info("Moved {} from {} to {}", region.getEncodedName(), sourceServer, targetServer);

Review comment:
       s/Moved/Requested Move
   
   since we are expressly not checking if it worked.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/util/RegionMover.java
##########
@@ -587,8 +483,12 @@ private void waitMoveTasksToFinish(ExecutorService moveRegionsPool,
         LOG.error("Interrupted while waiting for Thread to Complete " + e.getMessage(), e);
         throw e;
       } catch (ExecutionException e) {
-        LOG.error("Got Exception From Thread While moving region " + e.getMessage(), e);
-        throw e;
+        if (e.getCause() instanceof UnknownRegionException) {
+          LOG.info("Ignore unknown region, it might have been split/merged.");

Review comment:
       Include the name of the region in the log message.
   
   Also include the exception details at debug. otherwise if we get URE for some reason other than what we're expecting here it will be really hard for someone to figure out what is going on.

##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestRegionMover.java
##########
@@ -457,4 +463,37 @@ public void testExcludeAndDecomServers() throws Exception {
       Collections.emptyList());
   }
 
+  @Test
+  public void testWithMergedRegions() throws Exception {

Review comment:
       test with split regions as well?
   
   test that we properly fail in ack mode if the problem is something other than URE?




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