This is an automated email from the ASF dual-hosted git repository.
zhangduo pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/hbase.git
The following commit(s) were added to refs/heads/master by this push:
new 53e3aa902d2 HBASE-28984 Refactor cache update logic to use new
PermissionCache instances (#6485)
53e3aa902d2 is described below
commit 53e3aa902d234b17076a751c73a215464669e102
Author: Peiming Wu <[email protected]>
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[ 呉培銘 ] <[email protected]>
Signed-off-by: Duo Zhang <[email protected]>
---
.../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 c27063752c5..8f4b5a356b9 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
@@ -77,15 +77,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<>();
@@ -184,9 +175,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();
@@ -198,18 +187,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 58f99bc8fd7..da28f152c65 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);
+ }
}
