dennishuo commented on code in PR #2508:
URL: https://github.com/apache/polaris/pull/2508#discussion_r2326094632
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##########
@@ -164,12 +165,13 @@ Page<PolarisBaseEntity> loadEntities(
* @param entitySubType subType of entities to list (or ANY_SUBTYPE)
* @return list of all matching entities
*/
- default @Nonnull List<PolarisBaseEntity> loadEntitiesAll(
+ default @Nonnull List<PolarisBaseEntity> loadFullEntitiesAll(
Review Comment:
Should this be `listFullEntitiesAll` instead of `loadFullEntitiesAll`?
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##########
@@ -346,6 +348,21 @@ EntityResult loadEntity(
long entityId,
@Nonnull PolarisEntityType entityType);
+ /**
+ * Load a batch of entities given their {@link EntityNameLookupRecord}. Will
return an empty list
+ * if the input list is empty. Order in that returned list is the same as
the input list. Some
+ * elements might be NULL if the entity has been dropped.
+ *
+ * @param callCtx call context
+ * @param entityLookupRecords the list of entity lookup records to load
+ * @return a non-null list of entities corresponding to the lookup keys.
Some elements might be
+ * NULL if the entity has been dropped.
+ */
+ @Nonnull
+ EntitiesResult loadEntities(
Review Comment:
For the current main EntityCache architecture, generally anything that falls
into a "point lookup" is normally supposed to be cache-cooperative, while
"list" is fundamentally passthrough because the set of listed contents aren't
part of the entityVersion-controlled set (i.e. creation or modification of
child entities don't increment entityVersions on the parent entity).
In this way, we probably want "batch point-lookup" to align better with
Resolver and EntityCache, and make this `loadResolvedEntities` returning a
collection of `ResolvedEntities` instead of a collection of only the naked
`PolariseBaseEntity`.
Being able to resolve the grants all at once as well would be useful for
using this in the PrincipalRoles batch-lookup.
I'd be okay with having boolean input options to control whether the caller
actually wants to resolve grants as well in case we're concerned about doing
wasted work when callsites don't need the grants, so this method doesn't
necessarily need to be more expensive than only looking up naked entities.
But aligning the interface with ResolvedEntities could be a good step
towards making the Resolver/EntityCache interfaces less confusing and a better
match with the MetaStoreManager interface.
--
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]
