(hbase) branch branch-2.6 updated: HBASE-28984 Refactor cache update logic to use new PermissionCache instances (#6485)

[zh...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

zhangduo pushed a commit to branch branch-2.6
in repository https://gitbox.apache.org/repos/asf/hbase.git


The following commit(s) were added to refs/heads/branch-2.6 by this push:
     new 6e60998b434 HBASE-28984 Refactor cache update logic to use new PermissionCache instances (#6485)
6e60998b434 is described below

commit 6e60998b4343e865e0f3964d840b092712245ff6
Author: Peiming Wu <mk...@users.noreply.github.com>
AuthorDate: Mon Mar 17 17:04:36 2025 +0900

    HBASE-28984 Refactor cache update logic to use new PermissionCache instances (#6485)
    
    - This PR replaced the clear operation on existing PermissionCache with logic to create a new PermissionCache instance during updates.
    - This change ensures that readers have consistent and uninterrupted access to the cache data, minimizing the risk of race conditions.
    - By creating a new cache instance for each update instead of clearing the existing one, we improve data integrity and avoid potential issues related to concurrent access.
    - This approach aligns with the Copy-On-Write pattern, optimizing read performance by isolating update operations.
    
    Co-authored-by: Wu Peiming[ 呉培銘 ] <pe...@linecorp.com>
    Signed-off-by: Duo Zhang <zh...@apache.org>
    (cherry picked from commit 53e3aa902d234b17076a751c73a215464669e102)
---
 .../hadoop/hbase/security/access/AuthManager.java  | 21 +----
 .../security/access/TestZKPermissionWatcher.java   | 91 ++++++++++++++--------
 2 files changed, 62 insertions(+), 50 deletions(-)

diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AuthManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AuthManager.java
index 023eccbd27d..464c863d84a 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AuthManager.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AuthManager.java
@@ -76,15 +76,6 @@ public final class AuthManager {
         return cache.get(name);
       }
     }
-
-    void clear() {
-      synchronized (mutex) {
-        for (Map.Entry<String, Set<T>> entry : cache.entrySet()) {
-          entry.getValue().clear();
-        }
-        cache.clear();
-      }
-    }
   }
 
   PermissionCache<NamespacePermission> NS_NO_PERMISSION = new PermissionCache<>();
@@ -183,9 +174,7 @@ public final class AuthManager {
    * @param tablePerms new table permissions
    */
   private void updateTableCache(TableName table, ListMultimap<String, Permission> tablePerms) {
-    PermissionCache<TablePermission> cacheToUpdate =
-      tableCache.getOrDefault(table, new PermissionCache<>());
-    clearCache(cacheToUpdate);
+    PermissionCache<TablePermission> cacheToUpdate = new PermissionCache<>();
     updateCache(tablePerms, cacheToUpdate);
     tableCache.put(table, cacheToUpdate);
     mtime.incrementAndGet();
@@ -197,18 +186,12 @@ public final class AuthManager {
    * @param nsPerms   new namespace permissions
    */
   private void updateNamespaceCache(String namespace, ListMultimap<String, Permission> nsPerms) {
-    PermissionCache<NamespacePermission> cacheToUpdate =
-      namespaceCache.getOrDefault(namespace, new PermissionCache<>());
-    clearCache(cacheToUpdate);
+    PermissionCache<NamespacePermission> cacheToUpdate = new PermissionCache<>();
     updateCache(nsPerms, cacheToUpdate);
     namespaceCache.put(namespace, cacheToUpdate);
     mtime.incrementAndGet();
   }
 
-  private void clearCache(PermissionCache cacheToUpdate) {
-    cacheToUpdate.clear();
-  }
-
   @SuppressWarnings("unchecked")
   private void updateCache(ListMultimap<String, ? extends Permission> newPermissions,
     PermissionCache cacheToUpdate) {
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestZKPermissionWatcher.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestZKPermissionWatcher.java
index 788afc6c0e5..8b31db18713 100644
--- a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestZKPermissionWatcher.java
+++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestZKPermissionWatcher.java
@@ -107,6 +107,7 @@ public class TestZKPermissionWatcher {
     Configuration conf = UTIL.getConfiguration();
     User george = User.createUserForTesting(conf, "george", new String[] {});
     User hubert = User.createUserForTesting(conf, "hubert", new String[] {});
+    ListMultimap<String, UserPermission> permissions = ArrayListMultimap.create();
 
     assertFalse(AUTH_A.authorizeUserTable(george, TEST_TABLE, Permission.Action.READ));
     assertFalse(AUTH_A.authorizeUserTable(george, TEST_TABLE, Permission.Action.WRITE));
@@ -119,22 +120,9 @@ public class TestZKPermissionWatcher {
     assertFalse(AUTH_B.authorizeUserTable(hubert, TEST_TABLE, Permission.Action.WRITE));
 
     // update ACL: george RW
-    List<UserPermission> acl = new ArrayList<>(1);
-    acl.add(new UserPermission(george.getShortName(), Permission.newBuilder(TEST_TABLE)
-      .withActions(Permission.Action.READ, Permission.Action.WRITE).build()));
-    ListMultimap<String, UserPermission> multimap = ArrayListMultimap.create();
-    multimap.putAll(george.getShortName(), acl);
-    byte[] serialized = PermissionStorage.writePermissionsAsBytes(multimap, conf);
-    WATCHER_A.writeToZookeeper(TEST_TABLE.getName(), serialized);
-    final long mtimeB = AUTH_B.getMTime();
-    // Wait for the update to propagate
-    UTIL.waitFor(10000, 100, new Predicate<Exception>() {
-      @Override
-      public boolean evaluate() throws Exception {
-        return AUTH_B.getMTime() > mtimeB;
-      }
-    });
-    Thread.sleep(1000);
+    writeToZookeeper(WATCHER_A,
+      updatePermissions(permissions, george, Permission.Action.READ, Permission.Action.WRITE));
+    waitForModification(AUTH_B, 1000);
 
     // check it
     assertTrue(AUTH_A.authorizeUserTable(george, TEST_TABLE, Permission.Action.READ));
@@ -147,21 +135,8 @@ public class TestZKPermissionWatcher {
     assertFalse(AUTH_B.authorizeUserTable(hubert, TEST_TABLE, Permission.Action.WRITE));
 
     // update ACL: hubert R
-    List<UserPermission> acl2 = new ArrayList<>(1);
-    acl2.add(new UserPermission(hubert.getShortName(),
-      Permission.newBuilder(TEST_TABLE).withActions(TablePermission.Action.READ).build()));
-    final long mtimeA = AUTH_A.getMTime();
-    multimap.putAll(hubert.getShortName(), acl2);
-    byte[] serialized2 = PermissionStorage.writePermissionsAsBytes(multimap, conf);
-    WATCHER_B.writeToZookeeper(TEST_TABLE.getName(), serialized2);
-    // Wait for the update to propagate
-    UTIL.waitFor(10000, 100, new Predicate<Exception>() {
-      @Override
-      public boolean evaluate() throws Exception {
-        return AUTH_A.getMTime() > mtimeA;
-      }
-    });
-    Thread.sleep(1000);
+    writeToZookeeper(WATCHER_B, updatePermissions(permissions, hubert, Permission.Action.READ));
+    waitForModification(AUTH_A, 1000);
 
     // check it
     assertTrue(AUTH_A.authorizeUserTable(george, TEST_TABLE, Permission.Action.READ));
@@ -173,4 +148,58 @@ public class TestZKPermissionWatcher {
     assertTrue(AUTH_B.authorizeUserTable(hubert, TEST_TABLE, Permission.Action.READ));
     assertFalse(AUTH_B.authorizeUserTable(hubert, TEST_TABLE, Permission.Action.WRITE));
   }
+
+  @Test
+  public void testRaceConditionOnPermissionUpdate() throws Exception {
+    Configuration conf = UTIL.getConfiguration();
+    User george = User.createUserForTesting(conf, "george", new String[] {});
+    User hubert = User.createUserForTesting(conf, "hubert", new String[] {});
+    ListMultimap<String, UserPermission> permissions = ArrayListMultimap.create();
+
+    // update ACL: george RW
+    writeToZookeeper(WATCHER_A,
+      updatePermissions(permissions, george, Permission.Action.READ, Permission.Action.WRITE));
+    waitForModification(AUTH_A, 1000);
+
+    // check it
+    assertTrue(AUTH_A.authorizeUserTable(george, TEST_TABLE, Permission.Action.READ));
+    assertTrue(AUTH_A.authorizeUserTable(george, TEST_TABLE, Permission.Action.WRITE));
+
+    // update ACL: hubert A
+    writeToZookeeper(WATCHER_A, updatePermissions(permissions, hubert, Permission.Action.ADMIN));
+    // intended not to waitForModification(AUTH_A, 1000);
+    // check george permission should not be updated/removed while updating permission for hubert
+    for (int i = 0; i < 5000; i++) {
+      assertTrue(AUTH_A.authorizeUserTable(george, TEST_TABLE, Permission.Action.READ));
+      assertTrue(AUTH_A.authorizeUserTable(george, TEST_TABLE, Permission.Action.WRITE));
+    }
+  }
+
+  private ListMultimap<String, UserPermission> updatePermissions(
+    ListMultimap<String, UserPermission> permissions, User user, Permission.Action... actions) {
+    List<UserPermission> acl = new ArrayList<>(1);
+    acl.add(new UserPermission(user.getShortName(),
+      Permission.newBuilder(TEST_TABLE).withActions(actions).build()));
+    permissions.putAll(user.getShortName(), acl);
+    return permissions;
+  }
+
+  private void writeToZookeeper(ZKPermissionWatcher watcher,
+    ListMultimap<String, UserPermission> permissions) {
+    byte[] serialized =
+      PermissionStorage.writePermissionsAsBytes(permissions, UTIL.getConfiguration());
+    watcher.writeToZookeeper(TEST_TABLE.getName(), serialized);
+  }
+
+  private void waitForModification(AuthManager authManager, long sleep) throws Exception {
+    final long mtime = authManager.getMTime();
+    // Wait for the update to propagate
+    UTIL.waitFor(10000, 100, new Predicate<Exception>() {
+      @Override
+      public boolean evaluate() throws Exception {
+        return authManager.getMTime() > mtime;
+      }
+    });
+    Thread.sleep(sleep);
+  }
 }