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

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

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



##########
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.debug("Ignore unknown region, it might have been split/merged.");

Review comment:
       sure, let me use `info`

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/util/MoveWithAck.java
##########
@@ -0,0 +1,153 @@
+/*
+ *
+ * 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.HRegionLocation;
+import org.apache.hadoop.hbase.ServerName;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.Connection;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.hadoop.hbase.client.ResultScanner;
+import org.apache.hadoop.hbase.client.Scan;
+import org.apache.hadoop.hbase.client.Table;
+import org.apache.hadoop.hbase.filter.FirstKeyOnlyFilter;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.concurrent.Callable;
+
+/**
+ * Move Regions and make sure that they are up on the target server.If a region movement fails we
+ * exit as failure
+ */
+@InterfaceAudience.Private
+class MoveWithAck implements Callable<Boolean> {
+
+  private static final Logger LOG = LoggerFactory.getLogger(MoveWithAck.class);
+
+  private final RegionInfo region;
+  private final ServerName targetServer;
+  private final List<RegionInfo> movedRegions;
+  private final ServerName sourceServer;
+  private final Connection conn;
+  private final Admin admin;
+
+  MoveWithAck(Connection conn, RegionInfo regionInfo, ServerName sourceServer,
+    ServerName targetServer, List<RegionInfo> movedRegions) throws IOException {
+    this.conn = conn;
+    this.region = regionInfo;
+    this.targetServer = targetServer;
+    this.movedRegions = movedRegions;
+    this.sourceServer = sourceServer;
+    this.admin = conn.getAdmin();
+  }
+
+  @Override
+  public Boolean call() throws IOException, InterruptedException {
+    boolean moved = false;
+    int count = 0;
+    int retries = admin.getConfiguration()
+      .getInt(RegionMover.MOVE_RETRIES_MAX_KEY, RegionMover.DEFAULT_MOVE_RETRIES_MAX);
+    int maxWaitInSeconds = admin.getConfiguration()
+      .getInt(RegionMover.MOVE_WAIT_MAX_KEY, RegionMover.DEFAULT_MOVE_WAIT_MAX);
+    long startTime = EnvironmentEdgeManager.currentTime();
+    boolean sameServer = true;
+    // Assert we can scan the region in its current location
+    isSuccessfulScan(region);
+    LOG
+      .info("Moving region: {} from {} to {}", region.getEncodedName(), sourceServer, targetServer);
+    while (count < retries && sameServer) {
+      if (count > 0) {
+        LOG.info("Retry " + count + " of maximum " + retries);
+      }
+      count = count + 1;
+      admin.move(region.getEncodedNameAsBytes(), targetServer);
+      long maxWait = startTime + (maxWaitInSeconds * 1000);
+      while (EnvironmentEdgeManager.currentTime() < maxWait) {
+        sameServer = isSameServer(region, sourceServer);
+        if (!sameServer) {
+          break;
+        }
+        Thread.sleep(100);

Review comment:
       Agree, this was old code anyways, we can update this to 1 sec at least.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/util/MoveWithAck.java
##########
@@ -0,0 +1,153 @@
+/*
+ *
+ * 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.HRegionLocation;
+import org.apache.hadoop.hbase.ServerName;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.Connection;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.hadoop.hbase.client.ResultScanner;
+import org.apache.hadoop.hbase.client.Scan;
+import org.apache.hadoop.hbase.client.Table;
+import org.apache.hadoop.hbase.filter.FirstKeyOnlyFilter;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.concurrent.Callable;
+
+/**
+ * Move Regions and make sure that they are up on the target server.If a region movement fails we
+ * exit as failure
+ */
+@InterfaceAudience.Private
+class MoveWithAck implements Callable<Boolean> {
+
+  private static final Logger LOG = LoggerFactory.getLogger(MoveWithAck.class);
+
+  private final RegionInfo region;
+  private final ServerName targetServer;
+  private final List<RegionInfo> movedRegions;
+  private final ServerName sourceServer;
+  private final Connection conn;
+  private final Admin admin;
+
+  MoveWithAck(Connection conn, RegionInfo regionInfo, ServerName sourceServer,
+    ServerName targetServer, List<RegionInfo> movedRegions) throws IOException {
+    this.conn = conn;
+    this.region = regionInfo;
+    this.targetServer = targetServer;
+    this.movedRegions = movedRegions;
+    this.sourceServer = sourceServer;
+    this.admin = conn.getAdmin();
+  }
+
+  @Override
+  public Boolean call() throws IOException, InterruptedException {
+    boolean moved = false;
+    int count = 0;
+    int retries = admin.getConfiguration()
+      .getInt(RegionMover.MOVE_RETRIES_MAX_KEY, RegionMover.DEFAULT_MOVE_RETRIES_MAX);
+    int maxWaitInSeconds = admin.getConfiguration()
+      .getInt(RegionMover.MOVE_WAIT_MAX_KEY, RegionMover.DEFAULT_MOVE_WAIT_MAX);
+    long startTime = EnvironmentEdgeManager.currentTime();
+    boolean sameServer = true;
+    // Assert we can scan the region in its current location
+    isSuccessfulScan(region);
+    LOG
+      .info("Moving region: {} from {} to {}", region.getEncodedName(), sourceServer, targetServer);
+    while (count < retries && sameServer) {
+      if (count > 0) {
+        LOG.info("Retry " + count + " of maximum " + retries);
+      }
+      count = count + 1;
+      admin.move(region.getEncodedNameAsBytes(), targetServer);
+      long maxWait = startTime + (maxWaitInSeconds * 1000);
+      while (EnvironmentEdgeManager.currentTime() < maxWait) {
+        sameServer = isSameServer(region, sourceServer);
+        if (!sameServer) {
+          break;
+        }
+        Thread.sleep(100);
+      }
+    }
+    if (sameServer) {
+      LOG.error("Region: {} stuck on {} ,newServer={}", region.getRegionNameAsString(),
+        this.sourceServer, this.targetServer);
+    } else {
+      isSuccessfulScan(region);
+      LOG.info("Moved Region " + region.getRegionNameAsString() + " cost:" + String
+        .format("%.3f", (float) (EnvironmentEdgeManager.currentTime() - startTime) / 1000));
+      moved = true;
+      movedRegions.add(region);
+    }
+    return moved;
+  }
+
+  /**
+   * Tries to scan a row from passed region
+   */
+  private void isSuccessfulScan(RegionInfo region) throws IOException {
+    Scan scan = new Scan().withStartRow(region.getStartKey()).setRaw(true).setOneRowLimit()
+      .setMaxResultSize(1L).setCaching(1).setFilter(new FirstKeyOnlyFilter())
+      .setCacheBlocks(false);
+    try (Table table = conn.getTable(region.getTable());
+      ResultScanner scanner = table.getScanner(scan)) {
+      scanner.next();

Review comment:
       Yes, it is getting closed because we have wrapped it within try-with-resources above:
   ```
       try (Table table = conn.getTable(region.getTable());
         ResultScanner scanner = table.getScanner(scan)) {
   ```




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