Copilot commented on code in PR #9896:
URL: https://github.com/apache/gravitino/pull/9896#discussion_r2787499363
##########
server/src/test/java/org/apache/gravitino/server/web/rest/TestJobOperations.java:
##########
@@ -126,6 +129,11 @@ protected void configure() {
@BeforeAll
public static void setup() throws IllegalAccessException {
+ Config config = mock(Config.class);
+ org.mockito.Mockito.doReturn(false).when(config).get(CACHE_ENABLED);
+ org.mockito.Mockito.doReturn(false).when(config).get(ENABLE_AUTHORIZATION);
+ FieldUtils.writeField(GravitinoEnv.getInstance(), "config", config, true);
Review Comment:
In the test setup, `org.mockito.Mockito.doReturn(...)` is referenced via a
fully-qualified name even though Mockito is already used/imported elsewhere in
the file. Prefer using the imported `Mockito` (or existing static imports) to
keep the style consistent and avoid unnecessary FQNs in method bodies.
##########
core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java:
##########
@@ -315,12 +315,44 @@ public <E extends Entity & HasIdentifier> E get(
@Override
public <E extends Entity & HasIdentifier> List<E> batchGet(
List<NameIdentifier> identifiers, Entity.EntityType entityType) {
+ if (identifiers == null || identifiers.isEmpty()) {
+ return Lists.newArrayList();
+ }
+
+ // Verify all identifiers have the same namespace
+ Namespace firstNamespace = identifiers.get(0).namespace();
+ Preconditions.checkArgument(
+ identifiers.stream().allMatch(id ->
id.namespace().equals(firstNamespace)),
+ "All identifiers must have the same namespace for batch get
operation");
+
switch (entityType) {
+ case METALAKE:
+ return (List<E>)
+
MetalakeMetaService.getInstance().batchGetMetalakeByIdentifier(identifiers);
+ case CATALOG:
+ return (List<E>)
CatalogMetaService.getInstance().batchGetCatalogByIdentifier(identifiers);
+ case SCHEMA:
+ return (List<E>)
SchemaMetaService.getInstance().batchGetSchemaByIdentifier(identifiers);
case TABLE:
return (List<E>)
TableMetaService.getInstance().batchGetTableByIdentifier(identifiers);
+ case FILESET:
+ return (List<E>)
FilesetMetaService.getInstance().batchGetFilesetByIdentifier(identifiers);
+ case TOPIC:
+ return (List<E>)
TopicMetaService.getInstance().batchGetTopicByIdentifier(identifiers);
+ case MODEL:
+ return (List<E>)
ModelMetaService.getInstance().batchGetModelByIdentifier(identifiers);
+ case TAG:
+ return (List<E>)
TagMetaService.getInstance().batchGetTagByIdentifier(identifiers);
+ case POLICY:
+ return (List<E>)
PolicyMetaService.getInstance().batchGetPolicyByIdentifier(identifiers);
+ case JOB:
+ return (List<E>)
JobMetaService.getInstance().batchGetJobByIdentifier(identifiers);
+ case JOB_TEMPLATE:
+ return (List<E>)
+
JobTemplateMetaService.getInstance().batchGetJobTemplateByIdentifier(identifiers);
default:
throw new UnsupportedEntityTypeException(
- "Unsupported entity type: %s for get operation", entityType);
+ "Unsupported entity type: %s for batch get operation", entityType);
}
Review Comment:
New batch-get support was added for many entity types, including additional
input validation (non-empty list, same namespace) and new switch branches.
There don’t appear to be any relational-store tests exercising
`JDBCBackend.batchGet(...)` for the newly supported entity types (or the
namespace-mismatch/empty-list behaviors). Please add/extend tests (e.g., in the
existing `Test*MetaService`/`TestJDBCBackend` suite) to cover successful batch
gets for at least a couple of types and the validation/error paths.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java:
##########
@@ -328,12 +347,20 @@ private static void checkExecutor() {
private static void preloadToCache(
Entity.EntityType entityType, NameIdentifier[] nameIdentifiers) {
- if (GravitinoEnv.getInstance().cacheEnabled()) {
- if (entityType == Entity.EntityType.TABLE) {
- GravitinoEnv.getInstance()
- .entityStore()
- .batchGet(nameIdentifiers, entityType, TableEntity.class);
- }
+ if (!GravitinoEnv.getInstance().cacheEnabled()) {
+ return;
}
+
+ // Only preload entity types that support batch get operations
+ if (!SUPPORTED_PRELOAD_ENTITY_TYPES.contains(entityType)) {
+ return;
+ }
+
+ GravitinoEnv.getInstance()
+ .entityStore()
+ .batchGet(
+ Arrays.asList(nameIdentifiers),
+ entityType,
+ EntityClassMapper.getEntityClass(entityType));
}
Review Comment:
`preloadToCache` now has new behavior (supported-type filtering + batch
preloading via `entityStore().batchGet(...)`) but the existing
`TestMetadataAuthzHelper` suite does not appear to exercise this path (it never
enables cache). Consider adding a test that sets `GravitinoEnv.cacheEnabled()`
to true and asserts `entityStore().batchGet(...)` is invoked for a supported
entity type, and skipped for an unsupported type, to prevent regressions.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/MetalakeMetaService.java:
##########
@@ -397,4 +397,22 @@ public int deleteMetalakeMetasByLegacyTimeline(Long
legacyTimeline, int limit) {
mapper ->
mapper.deleteOwnerMetasByLegacyTimeline(legacyTimeline, limit)));
return metalakeDeleteCount[0] + ownerRelDeleteCount[0];
}
+
+ @Monitored(
+ metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
+ baseMetricName = "batchGetMetalakeByIdentifier")
+ public List<BaseMetalake> batchGetMetalakeByIdentifier(List<NameIdentifier>
identifiers) {
+
+ List<String> metalakeNames =
+ identifiers.stream()
+ .map(NameIdentifier::name)
+ .collect(java.util.stream.Collectors.toList());
+
Review Comment:
`batchGetMetalakeByIdentifier` uses a fully-qualified reference
`java.util.stream.Collectors.toList()` inside the method body. This diverges
from the codebase’s style guideline of using standard imports (and avoiding
FQNs unless needed for collision resolution). Prefer importing
`java.util.stream.Collectors` and using `Collectors.toList()` for consistency
and readability.
--
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]
