jerqi commented on code in PR #6455:
URL: https://github.com/apache/gravitino/pull/6455#discussion_r1955660178
##########
core/src/test/java/org/apache/gravitino/storage/relational/service/TestRoleMetaService.java:
##########
@@ -240,22 +241,29 @@ void insertRole() throws IOException {
SecurableObjects.ofTable(
schemaObject, "table",
Lists.newArrayList(Privileges.SelectTable.allow()));
+ // insert role
+ ArrayList<SecurableObject> securableObjects =
+ Lists.newArrayList(
+ catalogObject,
+ metalakeObject,
+ schemaObject,
+ filesetObject,
+ topicObject,
+ tableObject,
+ SecurableObjects.ofCatalog(
+ "anotherCatalog",
Lists.newArrayList(Privileges.UseCatalog.allow())));
+
+ // 对securableObjects按照fullName进行排序
Review Comment:
Could you use English comment instead of Chinese comment?
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/RoleMetaService.java:
##########
@@ -394,4 +443,49 @@ private static MetadataObject.Type getType(String type) {
private static String getEntityType(SecurableObject securableObject) {
return securableObject.type().name();
}
+
+ public static Map<Long, String> getMetadataObjectFullNames(Long metalakeId,
List<Long> ids) {
+ Map<Long, String> catalogIdAndNameMap = getCatalogIdAndNameMap(metalakeId);
+ Map<Long, Map<Long, String>> schemaIdAndNameMap =
getSchemaIdAndNameMap(metalakeId);
+
+ List<FilesetPO> filesetPOs =
+ SessionUtils.getWithoutCommit(
+ FilesetMetaMapper.class, mapper ->
mapper.listFilesetPOsByFilesetIds(ids));
+ if (filesetPOs == null || filesetPOs.isEmpty()) {
+ return new HashMap<>();
+ }
+
+ // 遍历filesetPOs，将其转换为Map
Review Comment:
Could you use English comment instead of Chinese comment?
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/RoleMetaService.java:
##########
@@ -394,4 +443,48 @@ private static MetadataObject.Type getType(String type) {
private static String getEntityType(SecurableObject securableObject) {
return securableObject.type().name();
}
+
+ public static Map<Long, String> getMetadataObjectFullNames(Long metalakeId,
List<Long> ids) {
+ Map<Long, String> catalogIdAndNameMap = getCatalogIdAndNameMap(metalakeId);
+ Map<Long, Map<Long, String>> schemaIdAndNameMap =
getSchemaIdAndNameMap(metalakeId);
Review Comment:
We don't need to get all the schema of the metalake. We can get the schema
id list according to the fileset list.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/RoleMetaService.java:
##########
@@ -394,4 +443,48 @@ private static MetadataObject.Type getType(String type) {
private static String getEntityType(SecurableObject securableObject) {
return securableObject.type().name();
}
+
+ public static Map<Long, String> getMetadataObjectFullNames(Long metalakeId,
List<Long> ids) {
+ Map<Long, String> catalogIdAndNameMap = getCatalogIdAndNameMap(metalakeId);
Review Comment:
We don't need to get all the catalogs of the metalake. We can get the
catalog id list according to the fileset list.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/RoleMetaService.java:
##########
@@ -353,21 +365,58 @@ private static List<SecurableObject>
listSecurableObjects(RolePO po) {
List<SecurableObjectPO> securableObjectPOs =
listSecurableObjectsByRoleId(po.getRoleId());
List<SecurableObject> securableObjects = Lists.newArrayList();
- for (SecurableObjectPO securableObjectPO : securableObjectPOs) {
- String fullName =
- MetadataObjectService.getMetadataObjectFullName(
- securableObjectPO.getType(),
securableObjectPO.getMetadataObjectId());
- if (fullName != null) {
- securableObjects.add(
- POConverters.fromSecurableObjectPO(
- fullName, securableObjectPO,
getType(securableObjectPO.getType())));
- } else {
- LOG.warn(
- "The securable object {} {} may be deleted",
- securableObjectPO.getMetadataObjectId(),
- securableObjectPO.getType());
- }
- }
+ securableObjectPOs.stream()
+ .collect(Collectors.groupingBy(SecurableObjectPO::getType))
+ .forEach(
+ (type, objects) -> {
+ // If the type is Fileset, use the batch retrieval interface;
+ // otherwise, use the single retrieval interface
Review Comment:
Maybe you can add `TODO` to get other securable objects using batch
retrieving?
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/RoleMetaService.java:
##########
@@ -353,21 +365,58 @@ private static List<SecurableObject>
listSecurableObjects(RolePO po) {
List<SecurableObjectPO> securableObjectPOs =
listSecurableObjectsByRoleId(po.getRoleId());
List<SecurableObject> securableObjects = Lists.newArrayList();
- for (SecurableObjectPO securableObjectPO : securableObjectPOs) {
- String fullName =
- MetadataObjectService.getMetadataObjectFullName(
- securableObjectPO.getType(),
securableObjectPO.getMetadataObjectId());
- if (fullName != null) {
- securableObjects.add(
- POConverters.fromSecurableObjectPO(
- fullName, securableObjectPO,
getType(securableObjectPO.getType())));
- } else {
- LOG.warn(
- "The securable object {} {} may be deleted",
- securableObjectPO.getMetadataObjectId(),
- securableObjectPO.getType());
- }
- }
+ securableObjectPOs.stream()
+ .collect(Collectors.groupingBy(SecurableObjectPO::getType))
+ .forEach(
+ (type, objects) -> {
+ // If the type is Fileset, use the batch retrieval interface;
+ // otherwise, use the single retrieval interface
+ if (type.equals(MetadataObject.Type.FILESET.name())) {
+ List<Long> filesetIds =
+ objects.stream()
+ .map(SecurableObjectPO::getMetadataObjectId)
+ .collect(Collectors.toList());
+
+ Map<Long, String> filesetIdAndNameMap =
+ getMetadataObjectFullNames(po.getMetalakeId(), filesetIds);
+
+ for (SecurableObjectPO securableObjectPO : objects) {
+ String fullName =
+
filesetIdAndNameMap.get(securableObjectPO.getMetadataObjectId());
+ if (fullName != null) {
+ securableObjects.add(
+ POConverters.fromSecurableObjectPO(
+ fullName, securableObjectPO,
getType(securableObjectPO.getType())));
+ } else {
+ LOG.warn(
+ "The securable object {} {} may be deleted",
+ securableObjectPO.getMetadataObjectId(),
+ securableObjectPO.getType());
+ }
+ }
+ } else {
+ for (SecurableObjectPO securableObjectPO : objects) {
+ String fullName =
+ MetadataObjectService.getMetadataObjectFullName(
+ securableObjectPO.getType(),
securableObjectPO.getMetadataObjectId());
+ if (fullName != null) {
+ securableObjects.add(
+ POConverters.fromSecurableObjectPO(
+ fullName, securableObjectPO,
getType(securableObjectPO.getType())));
+ } else {
+ LOG.warn(
+ "The securable object {} {} may be deleted",
+ securableObjectPO.getMetadataObjectId(),
+ securableObjectPO.getType());
+ }
+ }
+ }
+ });
+
+ // Since there are many comparisons of RoleEntity in the unit tests,
Review Comment:
```suggestion
// Since there are many comparisions of RoleEntity in the unit tests,
```
--
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]
