[#6238] improvement(storage): Improve get role performance when roles is bound to many metadata. #6455
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
In xiaomi company, we find when roles is bounded to many securable objects, then get role is very slow, so we try so solve get role function. |
FourFriends
force-pushed
the
feat-0214-role
branch
from
c811185 to
1860a7a
Compare
FourFriends
changed the title
jerqi
reviewed
Feb 14, 2025
core/src/main/java/org/apache/gravitino/storage/relational/service/RoleMetaService.java
Outdated
Show resolved
Hide resolved
| } | ||
| }); | ||
|
|
||
| // Since there are many comparisons of RoleEntity in the unit tests, |
There was a problem hiding this comment.
Suggested change
| // Since there are many comparisons of RoleEntity in the unit tests, | |
| // Since there are many comparisions of RoleEntity in the unit tests, |
core/src/test/java/org/apache/gravitino/storage/relational/service/TestRoleMetaService.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/storage/relational/service/RoleMetaService.java
Outdated
Show resolved
Hide resolved
|
|
||
| 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); |
There was a problem hiding this 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.
| .forEach( | ||
| (type, objects) -> { | ||
| // If the type is Fileset, use the batch retrieval interface; | ||
| // otherwise, use the single retrieval interface |
There was a problem hiding this comment.
Maybe you can add TODO to get other securable objects using batch retrieving?
jerqi
changed the title
yuqi1129
reviewed
Feb 14, 2025
| .collect(Collectors.toList()); | ||
|
|
||
| Map<Long, String> filesetIdAndNameMap = | ||
| getMetadataObjectFullNames(po.getMetalakeId(), filesetIds); |
There was a problem hiding this comment.
So the name should be getFilesetObjectFullNames, right?
| // Since there are many comparisons of RoleEntity in the unit tests, | ||
| // and the order after grouping by is different each time, | ||
| // the results are sorted by fullName here to ensure consistent query results. | ||
| securableObjects.sort(Comparator.comparing(MetadataObject::fullName)); |
There was a problem hiding this comment.
This is a temporary solution to make it work in UTs. It's the UTs that we should change. If there are no major performance issues, it's okay with me.
FourFriends
force-pushed
the
feat-0214-role
branch
from
1860a7a to
0dc985e
Compare
jerqi
reviewed
Feb 14, 2025
| @@ -35,6 +36,24 @@ public String listSchemaPOsByCatalogId(@Param("catalogId") Long catalogId) { | |||
| + " WHERE catalog_id = #{catalogId} AND deleted_at = 0"; | |||
| } | |||
|
|
|||
| public String listSchemaPOsByCatalogIds(@Param("catalogIds") List<Long> catalogIds) { | |||
There was a problem hiding this comment.
Maybe this should be listSchemaPOsBySchemaIds.
FourFriends
force-pushed
the
feat-0214-role
branch
from
0dc985e to
a638ad3
Compare
jerqi
reviewed
Feb 14, 2025
|
|
||
| filesetPOs.forEach( | ||
| filesetPO -> { | ||
| String catalogName = catalogIdAndNameMap.get(filesetPO.getCatalogId()); |
There was a problem hiding this comment.
Will catalogName or schemaName be null if catalog or schema is deleted?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
fix issue #6238
improve performance when a single role is bound to many metadata.
Why are the changes needed?
Use batch queries when getting role securable object full names instead of loop queries to get each securable object full name.
Fix: #6238
Does this PR introduce any user-facing change?
No
How was this patch tested?
Unit tests and integration tests have all passed, this feature has been running internally at Xiaomi for two weeks.