HBASE-30058 Eliminate unnecessary connection creation in snapshot operations (#8030) (#8314)

Each snapshot operation created two short-lived connections in
SnapshotDescriptionUtils.validate() — one for isSecurityAvailable()
to check hbase:acl table existence, and another in
writeAclToSnapshotDescription() to read ACL data. In Kerberos
environments with ZKConnectionRegistry, each connection triggered
a new ZK session with GSSAPI authentication and a TGS request to
the KDC, causing excessive KDC load during batch snapshot operations.

This patch reuses the caller's existing connection instead of
creating new ones:

- isSecurityAvailable() now accepts a Connection parameter
- writeAclToSnapshotDescription() passes the shared connection's
  Table to PermissionStorage.getTablePermissions()
- All callers (MasterRpcServices, RestoreSnapshotProcedure,
  CloneSnapshotProcedure) pass through their available connection

The original validate(SnapshotDescription, Configuration) and
isSecurityAvailable(Configuration) signatures are preserved as
overloads that delegate to the new Connection-based methods, so
existing callers (including tests) keep working unchanged. The
new Connection-based overloads are documented as preferred for
callers that already hold a Connection, to avoid the per-call
connection setup cost.

Co-authored-by: Jeongmin Ju <mini666@daum.net>

Signed-off-by: Junegunn Choi <junegunn@apache.org>

Author: junegunn

hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java

@@ -1701,8 +1701,8 @@ public SnapshotResponse snapshot(RpcController controller, SnapshotRequest reque
       LOG.info(master.getClientIdAuditPrefix() + " snapshot request for:"
         + ClientSnapshotDescriptionUtils.toString(request.getSnapshot()));
       // get the snapshot information
-      SnapshotDescription snapshot =
-        SnapshotDescriptionUtils.validate(request.getSnapshot(), master.getConfiguration());
+      SnapshotDescription snapshot = SnapshotDescriptionUtils.validate(master.getConnection(),
+        request.getSnapshot(), master.getConfiguration());
       // send back the max amount of time the client should wait for the snapshot to complete
       long waitTime = SnapshotDescriptionUtils.getMaxMasterTimeout(master.getConfiguration(),
         snapshot.getType(), SnapshotDescriptionUtils.DEFAULT_MAX_WAIT_TIME);

hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java

@@ -133,7 +133,7 @@ private void restoreSnapshotAcl(MasterProcedureEnv env) throws IOException {
     Configuration conf = env.getMasterServices().getConfiguration();
     if (
       restoreAcl && snapshot.hasUsersAndPermissions() && snapshot.getUsersAndPermissions() != null
-        && SnapshotDescriptionUtils.isSecurityAvailable(conf)
+        && SnapshotDescriptionUtils.isSecurityAvailable(env.getMasterServices().getConnection())
     ) {
       RestoreSnapshotHelper.restoreSnapshotAcl(snapshot, tableDescriptor.getTableName(), conf);
     }

hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java

@@ -555,7 +555,7 @@ private void addRegionsToInMemoryStates(List<RegionInfo> regionInfos, MasterProc
   private void restoreSnapshotAcl(final MasterProcedureEnv env) throws IOException {
     if (
       restoreAcl && snapshot.hasUsersAndPermissions() && snapshot.getUsersAndPermissions() != null
-        && SnapshotDescriptionUtils.isSecurityAvailable(env.getMasterServices().getConfiguration())
+        && SnapshotDescriptionUtils.isSecurityAvailable(env.getMasterServices().getConnection())
     ) {
       // restore acl of snapshot to table.
       RestoreSnapshotHelper.restoreSnapshotAcl(snapshot, TableName.valueOf(snapshot.getTable()),

hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/PermissionStorage.java

@@ -480,8 +480,13 @@ static Map<byte[], ListMultimap<String, UserPermission>> loadAll(Configuration c
 
   public static ListMultimap<String, UserPermission> getTablePermissions(Configuration conf,
     TableName tableName) throws IOException {
-    return getPermissions(conf, tableName != null ? tableName.getName() : null, null, null, null,
-      null, false);
+    return getTablePermissions(conf, tableName, null);
+  }
+
+  public static ListMultimap<String, UserPermission> getTablePermissions(Configuration conf,
+    TableName tableName, Table t) throws IOException {
+    return getPermissions(conf, tableName != null ? tableName.getName() : null, t, null, null, null,
+      false);
   }
 
   public static ListMultimap<String, UserPermission> getNamespacePermissions(Configuration conf,

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

@@ -34,6 +34,7 @@
 import org.apache.hadoop.hbase.client.Admin;
 import org.apache.hadoop.hbase.client.Connection;
 import org.apache.hadoop.hbase.client.ConnectionFactory;
+import org.apache.hadoop.hbase.client.Table;
 import org.apache.hadoop.hbase.ipc.RpcServer;
 import org.apache.hadoop.hbase.security.User;
 import org.apache.hadoop.hbase.security.access.AccessChecker;
@@ -317,10 +318,36 @@ private static Path getDefaultWorkingSnapshotDir(final Path rootDir) {
    */
   public static SnapshotDescription validate(SnapshotDescription snapshot, Configuration conf)
     throws IllegalArgumentException, IOException {
+    requireHasTable(snapshot);
+    try (Connection conn = ConnectionFactory.createConnection(conf)) {
+      return validate(conn, snapshot, conf);
+    }
+  }
+
+  private static void requireHasTable(SnapshotDescription snapshot) {
     if (!snapshot.hasTable()) {
       throw new IllegalArgumentException(
         "Descriptor doesn't apply to a table, so we can't build it.");
     }
+  }
+
+  /**
+   * Convert the passed snapshot description into a 'full' snapshot description based on default
+   * parameters, if none have been supplied. This resolves any 'optional' parameters that aren't
+   * supplied to their default values. Prefer this overload over
+   * {@link #validate(SnapshotDescription, Configuration)} when the caller already holds a
+   * {@link Connection}, to avoid the cost of creating a fresh connection (which involves a ZK
+   * session and, in Kerberos environments, a TGS request to the KDC) for each call.
+   * @param conn     connection to use for reading ACL information when security is enabled
+   * @param snapshot general snapshot descriptor
+   * @param conf     Configuration to read configured snapshot defaults if snapshot is not complete
+   * @return a valid snapshot description
+   * @throws IllegalArgumentException if the {@link SnapshotDescription} is not a complete
+   *                                  {@link SnapshotDescription}.
+   */
+  public static SnapshotDescription validate(Connection conn, SnapshotDescription snapshot,
+    Configuration conf) throws IllegalArgumentException, IOException {
+    requireHasTable(snapshot);
 
     SnapshotDescription.Builder builder = snapshot.toBuilder();
 
@@ -365,8 +392,8 @@ public static SnapshotDescription validate(SnapshotDescription snapshot, Configu
     snapshot = builder.build();
 
     // set the acl to snapshot if security feature is enabled.
-    if (isSecurityAvailable(conf)) {
-      snapshot = writeAclToSnapshotDescription(snapshot, conf);
+    if (isSecurityAvailable(conn)) {
+      snapshot = writeAclToSnapshotDescription(conn, snapshot, conf);
     }
     return snapshot;
   }
@@ -491,20 +518,32 @@ public static boolean isSnapshotOwner(org.apache.hadoop.hbase.client.SnapshotDes
   }
 
   public static boolean isSecurityAvailable(Configuration conf) throws IOException {
-    try (Connection conn = ConnectionFactory.createConnection(conf);
-      Admin admin = conn.getAdmin()) {
+    try (Connection conn = ConnectionFactory.createConnection(conf)) {
+      return isSecurityAvailable(conn);
+    }
+  }
+
+  /**
+   * Prefer this overload over {@link #isSecurityAvailable(Configuration)} when the caller already
+   * holds a {@link Connection}, to avoid the cost of creating a fresh connection (which involves a
+   * ZK session and, in Kerberos environments, a TGS request to the KDC) for each call.
+   */
+  public static boolean isSecurityAvailable(Connection conn) throws IOException {
+    try (Admin admin = conn.getAdmin()) {
       return admin.tableExists(PermissionStorage.ACL_TABLE_NAME);
     }
   }
 
-  private static SnapshotDescription writeAclToSnapshotDescription(SnapshotDescription snapshot,
-    Configuration conf) throws IOException {
+  private static SnapshotDescription writeAclToSnapshotDescription(Connection conn,
+    SnapshotDescription snapshot, Configuration conf) throws IOException {
     ListMultimap<String, UserPermission> perms =
       User.runAsLoginUser(new PrivilegedExceptionAction<ListMultimap<String, UserPermission>>() {
         @Override
         public ListMultimap<String, UserPermission> run() throws Exception {
-          return PermissionStorage.getTablePermissions(conf,
-            TableName.valueOf(snapshot.getTable()));
+          try (Table aclTable = conn.getTable(PermissionStorage.ACL_TABLE_NAME)) {
+            return PermissionStorage.getTablePermissions(conf,
+              TableName.valueOf(snapshot.getTable()), aclTable);
+          }
         }
       });
     return snapshot.toBuilder()