You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by "mini666 (via GitHub)" <gi...@apache.org> on 2026/04/08 01:27:15 UTC

[PR] HBASE-30058 Eliminate unnecessary connection creation in snapshot operations [hbase]

mini666 opened a new pull request, #8030:
URL: https://github.com/apache/hbase/pull/8030

   ## Summary
   
   Each snapshot operation created two short-lived connections in `SnapshotDescriptionUtils.validate()`:
   
   1. `isSecurityAvailable(conf)` — created a `Connection` + `Admin` just to check `hbase:acl` table existence
   2. `writeAclToSnapshotDescription()` — created another `Connection` to read ACL data from `hbase:acl`
   
   In Kerberos environments with the default `ZKConnectionRegistry`, each connection triggered a new ZK session with GSSAPI authentication and a TGS request to the KDC. During batch snapshot operations, this caused excessive KDC load that could lead to IP blocking.
   
   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
   
   Zero behavioral change — the same checks are performed, the same data is read, the same ACL is written to snapshots.
   
   See [HBASE-30058](https://issues.apache.org/jira/browse/HBASE-30058) for detailed root cause analysis including design ambiguity discussion.


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

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] HBASE-30058 Eliminate unnecessary connection creation in snapshot operations [hbase]

Posted by "junegunn (via GitHub)" <gi...@apache.org>.
junegunn commented on PR #8030:
URL: https://github.com/apache/hbase/pull/8030#issuecomment-4377227096

   Do we need to deprecate the original ones? We're still using them in multiple places. I think it's enough to mention that the new ones are for avoiding connection overhead.


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

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] HBASE-30058 Eliminate unnecessary connection creation in snapshot operations [hbase]

Posted by "mini666 (via GitHub)" <gi...@apache.org>.
mini666 commented on PR #8030:
URL: https://github.com/apache/hbase/pull/8030#issuecomment-4608173920

   @junegunn Sorry for the late reply.
   
   To be precise: **this patch isn't deployed to production yet** (it's not merged). What we rolled out to production was a *configuration* workaround — `hbase.client.registry.impl=RpcConnectionRegistry` — which addresses the same KDC-load symptom by a different mechanism: connections then authenticate under the server's shared UGI `Subject`, so the service ticket is cached and reused instead of re-fetched per connection.
   
   We don't have Grafana/ES charts for this, but we did capture `tcpdump` on the KDC port (88) before/after the workaround on one production HMaster (~228 tables) during a 5-minute snapshot-batch window:
   
   |  | Before (`ZKConnectionRegistry`) | After (`RpcConnectionRegistry`) |
   |---|---|---|
   | KDC TCP connections | 955 | 67 (**-93%**) |
   | `AS_REQ` (TGT) | 469 | 19 |
   | `TGS_REQ` (service ticket) | 486 | 48 |
   
   Before the change, a single KDC instance received ~486 requests in that window, which exceeded the KDC's rate limit and got the HMaster's IP blocked — the original symptom.
   
   One caveat on attribution: these numbers measure the **workaround**, not this patch directly. They do confirm that the snapshot `validate()` connection setup was the dominant source of KDC traffic — i.e. exactly the connections this patch removes. The difference is that the workaround *caches* tickets while this patch *eliminates* the redundant connections at the root, so this patch still helps deployments that keep the default `ZKConnectionRegistry`. I haven't produced a separate capture isolating only this patch's effect.


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

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] HBASE-30058 Eliminate unnecessary connection creation in snapshot operations [hbase]

Posted by "junegunn (via GitHub)" <gi...@apache.org>.
junegunn commented on PR #8030:
URL: https://github.com/apache/hbase/pull/8030#issuecomment-4414204116

   Thanks, LGTM. Have you deployed this to your production clusters? I'm wondering if you have any visual evidence (e.g., screenshots of Grafana charts) that shows the effectiveness of the patch.


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

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] HBASE-30058 Eliminate unnecessary connection creation in snapshot operations [hbase]

Posted by "junegunn (via GitHub)" <gi...@apache.org>.
junegunn merged PR #8030:
URL: https://github.com/apache/hbase/pull/8030


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

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] HBASE-30058 Eliminate unnecessary connection creation in snapshot operations [hbase]

Posted by "junegunn (via GitHub)" <gi...@apache.org>.
junegunn commented on code in PR #8030:
URL: https://github.com/apache/hbase/pull/8030#discussion_r3206318891


##########
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/PermissionStorage.java:
##########
@@ -486,6 +486,12 @@ public static ListMultimap<String, UserPermission> getTablePermissions(Configura
       null, false);
   }
 
+  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);
+  }

Review Comment:
   Please take a look at the test failure.



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

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] HBASE-30058 Eliminate unnecessary connection creation in snapshot operations [hbase]

Posted by "mini666 (via GitHub)" <gi...@apache.org>.
mini666 commented on PR #8030:
URL: https://github.com/apache/hbase/pull/8030#issuecomment-4411027063

   @junegunn Pushed an amended commit `a24eb390bf`.
   
   The failure was in `TestSnapshotDescriptionUtils#testValidateMissingTableName`, which expects `validate(SnapshotDescription, Configuration)` to throw `IllegalArgumentException` immediately when the description has no table, without performing any I/O — a property the previous master code had.
   
   After this PR, the new wrapper called `ConnectionFactory.createConnection(conf)` first, which eagerly bootstraps a master RPC and fails with `ConnectException` (against a stub `Configuration(false)`) before the inner `hasTable()` check in the new 3-arg overload could throw.
   
   The fix is to do the same fail-fast check in the wrapper before paying the connection setup cost. Verified locally:
   
   - `TestSnapshotDescriptionUtils` 5/5
   - `TestSnapshotManager` 4/4
   - `TestSnapshotFromMaster` 8/8
   
   The `large-wave-1` failures from the previous run (`TestHRegion` parallel tests, `TestEditsBehindDroppedTableTiming`) appear unrelated to this PR's scope and look like known flaky tests; expect them to clear on the rerun.


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

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] HBASE-30058 Eliminate unnecessary connection creation in snapshot operations [hbase]

Posted by "mini666 (via GitHub)" <gi...@apache.org>.
mini666 commented on code in PR #8030:
URL: https://github.com/apache/hbase/pull/8030#discussion_r3193248425


##########
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/PermissionStorage.java:
##########
@@ -486,6 +486,12 @@ public static ListMultimap<String, UserPermission> getTablePermissions(Configura
       null, false);
   }
 
+  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);
+  }

Review Comment:
   Good catch — done. The 2-arg overload now delegates to the 3-arg one with `t=null` to avoid duplicating the `getPermissions(...)` call.



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

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] HBASE-30058 Eliminate unnecessary connection creation in snapshot operations [hbase]

Posted by "junegunn (via GitHub)" <gi...@apache.org>.
junegunn commented on PR #8030:
URL: https://github.com/apache/hbase/pull/8030#issuecomment-4411303157

   Thanks, please avoid amending and force-pushing when addressing comments as it makes it hard to review your claims.


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

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] HBASE-30058 Eliminate unnecessary connection creation in snapshot operations [hbase]

Posted by "mini666 (via GitHub)" <gi...@apache.org>.
mini666 commented on PR #8030:
URL: https://github.com/apache/hbase/pull/8030#issuecomment-4414045963

   @junegunn Pushed `cc167d9295` (as a new commit on top of `a24eb390bf`):
   
   1. Extracted the `hasTable()` check into a private `requireHasTable(SnapshotDescription)` helper, called from both `validate(SnapshotDescription, Configuration)` and `validate(Connection, SnapshotDescription, Configuration)`.
   2. Dropped the over-explaining comment.
   
   Verified locally: `TestSnapshotDescriptionUtils` 5/5, `TestSnapshotManager` 4/4, `TestSnapshotFromMaster` 8/8 still pass.
   
   The previous run is currently in `action_required` state — would you mind approving the workflow run when you have a moment?


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

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] HBASE-30058 Eliminate unnecessary connection creation in snapshot operations [hbase]

Posted by "junegunn (via GitHub)" <gi...@apache.org>.
junegunn commented on PR #8030:
URL: https://github.com/apache/hbase/pull/8030#issuecomment-4629374933

   Merged, thanks! I opened backport PRs and will merge them once CI passes.


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

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] HBASE-30058 Eliminate unnecessary connection creation in snapshot operations [hbase]

Posted by "junegunn (via GitHub)" <gi...@apache.org>.
junegunn commented on PR #8030:
URL: https://github.com/apache/hbase/pull/8030#issuecomment-4589177191

   @mini666 
   
   > Thanks, LGTM. Have you deployed this to your production clusters? I'm wondering if you have any visual evidence (e.g., screenshots of Grafana/ES charts) that shows the effectiveness of the patch.
   
   Any comments?


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

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] HBASE-30058 Eliminate unnecessary connection creation in snapshot operations [hbase]

Posted by "junegunn (via GitHub)" <gi...@apache.org>.
junegunn commented on code in PR #8030:
URL: https://github.com/apache/hbase/pull/8030#discussion_r3212928816


##########
hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java:
##########
@@ -302,6 +303,35 @@ private static Path getDefaultWorkingSnapshotDir(final Path rootDir) {
    */
   public static SnapshotDescription validate(SnapshotDescription snapshot, Configuration conf)
     throws IllegalArgumentException, IOException {
+    // Fail fast on the basic invariant before paying the cost of creating a Connection (which
+    // involves a ZK session and, in Kerberos environments, a TGS request to the KDC). This also
+    // preserves the prior behavior of this overload, which callers (including unit tests with
+    // stub Configurations) rely on to throw without performing any I/O.
+    if (!snapshot.hasTable()) {
+      throw new IllegalArgumentException(
+        "Descriptor doesn't apply to a table, so we can't build it.");
+    }

Review Comment:
   1. Let's extract the `hasTable` check to avoid duplication.
   2. Drop the comment at lines 306-309; it over-explains.



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

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] HBASE-30058 Eliminate unnecessary connection creation in snapshot operations [hbase]

Posted by "mini666 (via GitHub)" <gi...@apache.org>.
mini666 commented on PR #8030:
URL: https://github.com/apache/hbase/pull/8030#issuecomment-4384818661

   @junegunn Good point — agreed. Dropped `@Deprecated` from the original `validate(SnapshotDescription, Configuration)` and `isSecurityAvailable(Configuration)` overloads, and added a Javadoc note on the new `Connection`-based overloads explaining they're preferred when the caller already holds a `Connection` (to avoid the per-call connection setup cost). Updated the commit message accordingly.


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

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] HBASE-30058 Eliminate unnecessary connection creation in snapshot operations [hbase]

Posted by "mini666 (via GitHub)" <gi...@apache.org>.
mini666 commented on PR #8030:
URL: https://github.com/apache/hbase/pull/8030#issuecomment-4376881323

   @junegunn Thanks for the review! Updated:
   
   1. **Overloads:** Kept the original `validate(SnapshotDescription, Configuration)` and `isSecurityAvailable(Configuration)` signatures as `@Deprecated` overloads that delegate to the new `Connection`-based methods. Reverted the test file changes since they no longer need the `null` Connection workaround.
   2. **Commit message:** Dropped the `Signed-off-by` line.
   
   Diff is now down to 5 production files (no test changes). Please take another look when you get a chance.


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

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] HBASE-30058 Eliminate unnecessary connection creation in snapshot operations [hbase]

Posted by "junegunn (via GitHub)" <gi...@apache.org>.
junegunn commented on code in PR #8030:
URL: https://github.com/apache/hbase/pull/8030#discussion_r3193128027


##########
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/PermissionStorage.java:
##########
@@ -486,6 +486,12 @@ public static ListMultimap<String, UserPermission> getTablePermissions(Configura
       null, false);
   }
 
+  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);
+  }

Review Comment:
   The existing one without `t` can call this:
   
   ```java
     public static ListMultimap<String, UserPermission> getTablePermissions(Configuration conf,
       TableName tableName) throws IOException {
       return getTablePermissions(conf, tableName, null);
     }
   ```



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

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org