yuqi1129 commented on code in PR #13006:
URL: https://github.com/apache/gravitino/pull/13006#discussion_r4072531198
##########
core/src/main/java/org/apache/gravitino/authorization/PermissionManager.java:
##########
@@ -870,6 +859,30 @@ public void execute() {
}
}
+ private List<RoleEntity> getExistingRoles(
+ String metalake, @Nullable List<String> roleNames, @Nullable List<Long>
roleIds) {
+ List<RoleEntity> roles = Lists.newArrayList();
+ if (roleNames == null || roleNames.isEmpty()) {
+ return roles;
+ }
+ if (roleIds == null || roleNames.size() != roleIds.size()) {
+ throw new IllegalRoleException("Existing role names and IDs must be
paired");
+ }
+ for (int i = 0; i < roleNames.size(); i++) {
+ RoleEntity role = roleManager.getRole(metalake, roleNames.get(i));
Review Comment:
> Should we batch get roles here? It seems to benefit the transaction.
Good catch — it turned out these reads shouldn't happen at all, so I removed
them rather than batching them.
`getExistingRoles` ran inside the `store.update(...)` updater, which
`JDBCBackend.update:225-228` already wraps in a transaction. Inside that
transaction `UserMetaService.updateUser:243` (and
`GroupMetaService.updateGroup`) builds the snapshot from
`RoleMetaService.listRolesByUserId(...)` — a single join over `user_role_rel`.
So `userEntity.roleNames()` and `roleIds()` are *already* one batched read,
with every name paired to the ID that produced it. Re-resolving those names one
row at a time only added N round trips to the transaction.
Worse, it re-resolved them **by name**, which is exactly the path that lets
a deleted-and-recreated role inherit an observed membership. The previous
commit guarded that with an ID-mismatch rejection; carrying the observed IDs
forward removes the hazard instead of detecting it, which is the option
@jerryshao suggested first in the thread above.
`f202d44` therefore drops `getExistingRoles`/`toRoleIds`/`toRoleNames` and
keeps only `checkObservedRoles`, the free length/pairing check (role IDs are an
optional entity field). Existing-role reads inside the transaction go from N to
0, and the "stale IDs never retarget same-name replacements" guarantee is now
structural.
`testMembershipUpdateRejectsChangedExistingRole` is replaced by
`testMembershipUpdateCarriesObservedRolesForward`, which asserts the
replacement is not inherited *and* `verify(roles, never()).getRole(METALAKE,
"retained")`. `testMembershipUpdateRequiresPairedRoleIds` and
`testRevokeOldRoleDoesNotRemoveReplacementName` are unchanged and still pass.
I did consider a real `name IN (...)` batch, but it needs a new batch-get on
the `EntityStore` SPI (`RoleMetaMapper` only has the single-row
`selectRoleMetaByMetalakeIdAndName`), and the no-SPI alternative
`store.list(roleNamespace)` would scan every role in the metalake to serve a
user who typically holds a handful.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
