xunliu commented on code in PR #6770:
URL: https://github.com/apache/gravitino/pull/6770#discussion_r2018047084
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java:
##########
@@ -761,18 +772,36 @@ public Boolean onGroupAdded(Group group) throws
AuthorizationPluginException {
@Override
public Boolean onGroupRemoved(Group group) throws
AuthorizationPluginException {
- VXGroupList list = rangerClient.searchGroup(ImmutableMap.of("name",
group.name()));
- if (list.getListSize() == 0) {
- LOG.warn("The group({}) doesn't exist in the Ranger!", group);
+ VXGroupList vxGroupList = rangerClient.searchGroup(ImmutableMap.of("name",
group.name()));
+ boolean isExist = false;
+ long groupId = -1;
Review Comment:
I think maybe we can use LONG groupId or Option(long) groupId to replace
`long`
##########
api/src/main/java/org/apache/gravitino/authorization/MetadataObjectChange.java:
##########
@@ -41,6 +41,19 @@ static MetadataObjectChange rename(
return new RenameMetadataObject(metadataObject, newMetadataObject);
}
+ /**
+ * Rename a metadata entity MetadataObjectChange.
+ *
+ * @param metadataObject The metadata object.
+ * @param newMetadataObject The new metadata object.
+ * @param locations The locations of the metadata object.
+ * @return return a MetadataObjectChange for the rename metadata object.
+ */
+ static MetadataObjectChange rename(
+ MetadataObject metadataObject, MetadataObject newMetadataObject,
List<String> locations) {
Review Comment:
I think we can use this new function to replace `static MetadataObjectChange
rename(MetadataObject metadataObject, MetadataObject newMetadataObject)` old
function.
We try to keep the function interface as small and concise as possible.
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java:
##########
@@ -735,18 +735,29 @@ public Boolean onUserAdded(User user) throws
AuthorizationPluginException {
@Override
public Boolean onUserRemoved(User user) throws AuthorizationPluginException {
VXUserList list = rangerClient.searchUser(ImmutableMap.of("name",
user.name()));
- if (list.getListSize() == 0) {
- LOG.warn("The user({}) doesn't exist in the Ranger!", user);
+ boolean isExist = false;
+ long userId = 0;
Review Comment:
I think using two variables to manage `userId` is difficult.
Maybe we can use a LONG type of user ID?
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java:
##########
@@ -761,18 +772,36 @@ public Boolean onGroupAdded(Group group) throws
AuthorizationPluginException {
@Override
public Boolean onGroupRemoved(Group group) throws
AuthorizationPluginException {
- VXGroupList list = rangerClient.searchGroup(ImmutableMap.of("name",
group.name()));
- if (list.getListSize() == 0) {
- LOG.warn("The group({}) doesn't exist in the Ranger!", group);
+ VXGroupList vxGroupList = rangerClient.searchGroup(ImmutableMap.of("name",
group.name()));
+ boolean isExist = false;
+ long groupId = -1;
+ try {
+ for (VXGroup vxGroup : vxGroupList.getList()) {
+ Class<?> clazz = vxGroup.getClass();
+ Field field = clazz.getDeclaredField("name");
+ field.setAccessible(true);
+ String value = (String) field.get(vxGroup);
+ if (group.name().equals(value)) {
+ groupId = vxGroup.getId();
+ isExist = true;
+ break;
+ }
+ }
+ } catch (Exception e) {
+ throw new AuthorizationPluginException("Fail to get the field name of
class VXGroup");
+ }
Review Comment:
I think we can reuse `isGroupExist()` function. If we can return
Option(long) or Long type in the `isGroupExist` function?
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationHDFSPlugin.java:
##########
@@ -665,8 +690,43 @@ public Boolean onMetadataUpdated(MetadataObjectChange...
changes) throws Runtime
// Did not need to update the Ranger policy
continue;
}
- List<AuthorizationMetadataObject> oldAuthzMetadataObjects =
- translateMetadataObject(metadataObject);
+
+ // If locations don't change, we don't need to modify the policies
+ if (renameChange.locations() == null ||
renameChange.locations().isEmpty()) {
+ continue;
+ }
Review Comment:
I think maybe we need to check if locations have changed?
--
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: commits-unsubscr...@gravitino.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
