This is an automated email from the ASF dual-hosted git repository.
junegunn pushed a commit to branch branch-2.5
in repository https://gitbox.apache.org/repos/asf/hbase.git
The following commit(s) were added to refs/heads/branch-2.5 by this push:
new 1df0968d7f9 HBASE-30058 Eliminate unnecessary connection creation in
snapshot operations (#8030) (#8315)
1df0968d7f9 is described below
commit 1df0968d7f9f1f3a658cc65625210ca0863f11e2
Author: Junegunn Choi <[email protected]>
AuthorDate: Sat Jun 6 09:21:35 2026 +0900
HBASE-30058 Eliminate unnecessary connection creation in snapshot
operations (#8030) (#8315)
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 <[email protected]>
Signed-off-by: Junegunn Choi <[email protected]>
---
.../hadoop/hbase/master/MasterRpcServices.java | 4 +-
.../master/procedure/CloneSnapshotProcedure.java | 2 +-
.../master/procedure/RestoreSnapshotProcedure.java | 2 +-
.../hbase/security/access/PermissionStorage.java | 9 +++-
.../hbase/snapshot/SnapshotDescriptionUtils.java | 55 ++++++++++++++++++----
5 files changed, 58 insertions(+), 14 deletions(-)
diff --git
a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java
b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java
index c31f116a089..c639e8eb91e 100644
---
a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java
+++
b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java
@@ -1619,8 +1619,8 @@ public class MasterRpcServices extends RSRpcServices
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());
master.snapshotManager.takeSnapshot(snapshot);
// send back the max amount of time the client should wait for the
snapshot to complete
diff --git
a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java
b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java
index b59aaaf4de9..18205284e65 100644
---
a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java
+++
b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java
@@ -132,7 +132,7 @@ public class CloneSnapshotProcedure extends
AbstractStateMachineTableProcedure<C
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);
}
diff --git
a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java
b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java
index cdc5f5ab3a2..85dd90fac2e 100644
---
a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java
+++
b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java
@@ -532,7 +532,7 @@ public class RestoreSnapshotProcedure
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()),
diff --git
a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/PermissionStorage.java
b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/PermissionStorage.java
index d9fdd0273e7..918eb685685 100644
---
a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/PermissionStorage.java
+++
b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/PermissionStorage.java
@@ -480,8 +480,13 @@ public final class PermissionStorage {
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,
diff --git
a/hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java
b/hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java
index fbb3cb37902..45a1d77f3fd 100644
---
a/hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java
+++
b/hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java
@@ -34,6 +34,7 @@ import org.apache.hadoop.hbase.TableName;
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.security.User;
import org.apache.hadoop.hbase.security.access.PermissionStorage;
import org.apache.hadoop.hbase.security.access.ShadedAccessControlUtil;
@@ -304,10 +305,36 @@ public final class SnapshotDescriptionUtils {
*/
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);
// set the creation time, if one hasn't been set
long time = snapshot.getCreationTime();
@@ -339,8 +366,8 @@ public final class SnapshotDescriptionUtils {
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;
}
@@ -465,20 +492,32 @@ public final class SnapshotDescriptionUtils {
}
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()
