HBASE-30248 Validate snapshot region timeout

hbase-common/src/main/resources/hbase-default.xml

@@ -1964,6 +1964,7 @@ possible configurations would overwhelm and obscure the important.
     <value>300000</value>
     <description>
        Timeout for regionservers to keep threads in snapshot request pool waiting.
+       Must be greater than 0.
     </description>
    </property>
    <property>

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/snapshot/RegionServerSnapshotManager.java

@@ -86,7 +86,10 @@ public class RegionServerSnapshotManager extends RegionServerProcedureManager {
   /** # of threads for snapshotting regions on the rs. */
   public static final int SNAPSHOT_REQUEST_THREADS_DEFAULT = 10;
 
-  /** Conf key for max time to keep threads in snapshot request pool waiting */
+  /**
+   * Conf key for max time to keep threads in snapshot request pool waiting. The configured value
+   * must be greater than 0.
+   */
   public static final String SNAPSHOT_TIMEOUT_MILLIS_KEY = "hbase.snapshot.region.timeout";
   /** Keep threads alive in request pool for max of 300 seconds */
   public static final long SNAPSHOT_TIMEOUT_MILLIS_DEFAULT = 5 * 60000;
@@ -101,6 +104,16 @@ public class RegionServerSnapshotManager extends RegionServerProcedureManager {
   private ProcedureMemberRpcs memberRpcs;
   private ProcedureMember member;
 
+  static long getSnapshotTimeoutMillis(Configuration conf) {
+    long timeoutMillis = conf.getLong(SNAPSHOT_TIMEOUT_MILLIS_KEY, SNAPSHOT_TIMEOUT_MILLIS_DEFAULT);
+    if (timeoutMillis > 0) {
+      return timeoutMillis;
+    }
+    LOG.warn("{} must be greater than 0, but is {}. Using default value {}",
+      SNAPSHOT_TIMEOUT_MILLIS_KEY, timeoutMillis, SNAPSHOT_TIMEOUT_MILLIS_DEFAULT);
+    return SNAPSHOT_TIMEOUT_MILLIS_DEFAULT;
+  }
+
   /**
    * Exposed for testing.
    * @param conf       HBase configuration.
@@ -176,7 +189,7 @@ public Subprocedure buildSubprocedure(SnapshotDescription snapshot) {
       + snapshot.getTable() + " type " + snapshot.getType());
     ForeignExceptionDispatcher exnDispatcher = new ForeignExceptionDispatcher(snapshot.getName());
     Configuration conf = rss.getConfiguration();
-    long timeoutMillis = conf.getLong(SNAPSHOT_TIMEOUT_MILLIS_KEY, SNAPSHOT_TIMEOUT_MILLIS_DEFAULT);
+    long timeoutMillis = getSnapshotTimeoutMillis(conf);
     long wakeMillis =
       conf.getLong(SNAPSHOT_REQUEST_WAKE_MILLIS_KEY, SNAPSHOT_REQUEST_WAKE_MILLIS_DEFAULT);
 
@@ -267,8 +280,7 @@ static class SnapshotSubprocedurePool {
     SnapshotSubprocedurePool(String name, Configuration conf, Abortable abortable) {
       this.abortable = abortable;
       // configure the executor service
-      long keepAlive = conf.getLong(RegionServerSnapshotManager.SNAPSHOT_TIMEOUT_MILLIS_KEY,
-        RegionServerSnapshotManager.SNAPSHOT_TIMEOUT_MILLIS_DEFAULT);
+      long keepAlive = getSnapshotTimeoutMillis(conf);
       int threads = conf.getInt(CONCURENT_SNAPSHOT_TASKS_KEY, DEFAULT_CONCURRENT_SNAPSHOT_TASKS);
       this.name = name;
       executor = Threads.getBoundedCachedThreadPool(threads, keepAlive, TimeUnit.MILLISECONDS,
@@ -383,7 +395,7 @@ public void initialize(RegionServerServices rss) throws KeeperException {
 
     // read in the snapshot request configuration properties
     Configuration conf = rss.getConfiguration();
-    long keepAlive = conf.getLong(SNAPSHOT_TIMEOUT_MILLIS_KEY, SNAPSHOT_TIMEOUT_MILLIS_DEFAULT);
+    long keepAlive = getSnapshotTimeoutMillis(conf);
     int opThreads = conf.getInt(SNAPSHOT_REQUEST_THREADS_KEY, SNAPSHOT_REQUEST_THREADS_DEFAULT);
 
     // create the actual snapshot procedure member

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/snapshot/TestRegionServerSnapshotManager.java

@@ -0,0 +1,47 @@
+/*
+ * 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.regionserver.snapshot;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.testclassification.RegionServerTests;
+import org.apache.hadoop.hbase.testclassification.SmallTests;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+
+@Tag(RegionServerTests.TAG)
+@Tag(SmallTests.TAG)
+public class TestRegionServerSnapshotManager {
+
+  @Test
+  public void testSnapshotTimeoutMustBePositive() {
+    Configuration conf = new Configuration(false);
+
+    conf.setLong(RegionServerSnapshotManager.SNAPSHOT_TIMEOUT_MILLIS_KEY, -1);
+    assertEquals(RegionServerSnapshotManager.SNAPSHOT_TIMEOUT_MILLIS_DEFAULT,
+      RegionServerSnapshotManager.getSnapshotTimeoutMillis(conf));
+
+    conf.setLong(RegionServerSnapshotManager.SNAPSHOT_TIMEOUT_MILLIS_KEY, 0);
+    assertEquals(RegionServerSnapshotManager.SNAPSHOT_TIMEOUT_MILLIS_DEFAULT,
+      RegionServerSnapshotManager.getSnapshotTimeoutMillis(conf));
+
+    conf.setLong(RegionServerSnapshotManager.SNAPSHOT_TIMEOUT_MILLIS_KEY, 1);
+    assertEquals(1, RegionServerSnapshotManager.getSnapshotTimeoutMillis(conf));
+  }
+}