Copilot commented on code in PR #9283:
URL: https://github.com/apache/gravitino/pull/9283#discussion_r2570905757
##########
server/src/main/java/org/apache/gravitino/server/web/rest/JobOperations.java:
##########
@@ -98,8 +107,19 @@ public Response listJobTemplates(
return Utils.doAs(
httpRequest,
() -> {
- List<JobTemplateDTO> jobTemplates =
-
toJobTemplateDTOs(jobOperationDispatcher.listJobTemplates(metalake));
+ List<JobTemplateEntity> jobTemplateEntities =
+ Lists.newArrayList(
+ MetadataAuthzHelper.filterByExpression(
+ metalake,
+ "METALAKE::OWNER || JOB_TEMPLATE::OWNER ||
ANY_USE_JOB_TEMPLATE",
+ Entity.EntityType.JOB_TEMPLATE,
+ jobOperationDispatcher
+ .listJobTemplates(metalake)
+ .toArray(new JobTemplateEntity[0]),
+ jobTemplateEntity ->
+ NameIdentifierUtil.ofJobTemplate(metalake,
jobTemplateEntity.name())));
Review Comment:
Converting a list to an array and back to a list is inefficient. Consider
passing the list directly to `filterByExpression` if possible, or use
`Iterables.toArray()` to avoid an intermediate list copy from the dispatcher.
##########
server/src/main/java/org/apache/gravitino/server/web/rest/JobOperations.java:
##########
@@ -258,7 +295,15 @@ public Response listJobs(
httpRequest,
() -> {
List<JobEntity> jobEntities =
- jobOperationDispatcher.listJobs(metalake,
Optional.ofNullable(jobTemplateName));
+ Lists.newArrayList(
+ MetadataAuthzHelper.filterByExpression(
+ metalake,
+ "METALAKE::OWNER || JOB::OWNER",
+ Entity.EntityType.JOB,
+ jobOperationDispatcher
+ .listJobs(metalake,
Optional.ofNullable(jobTemplateName))
+ .toArray(new JobEntity[0]),
+ jobEntity -> NameIdentifierUtil.ofJob(metalake,
jobEntity.name())));
Review Comment:
Same inefficiency as comment 1: converting list to array then back to list.
Consider refactoring to avoid the double conversion.
```suggestion
MetadataAuthzHelper.filterByExpression(
metalake,
"METALAKE::OWNER || JOB::OWNER",
Entity.EntityType.JOB,
jobOperationDispatcher
.listJobs(metalake,
Optional.ofNullable(jobTemplateName)),
jobEntity -> NameIdentifierUtil.ofJob(metalake,
jobEntity.name()));
```
##########
core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java:
##########
@@ -196,21 +204,19 @@ public static void checkMetadataObject(String metalake,
MetadataObject object) {
try {
env.accessControlDispatcher().getRole(metalake, object.fullName());
} catch (NoSuchRoleException nsr) {
- Preconditions.checkArgument(
- exceptionToThrowSupplier != null, "exceptionToThrowSupplier
should not be null");
throw exceptionToThrowSupplier.get();
Review Comment:
Removed null check for `exceptionToThrowSupplier` that was present in the
original code. While this may be intentional based on context elsewhere,
consider whether this removal could cause issues if the supplier is null, or
add a comment explaining why the check is no longer needed.
```suggestion
throw checkNotNull(exceptionToThrowSupplier).get();
```
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/MetadataObjectService.java:
##########
@@ -104,9 +102,39 @@ private static Map<Long, String>
getPolicyObjectsFullName(List<Long> policyIds)
policyMetaMapper.selectPolicyByPolicyId(policyId).getPolicyName())));
}
+ private static Map<Long, String> getJobObjectsFullName(List<Long> jobIds) {
+ if (jobIds == null || jobIds.isEmpty()) {
+ return Maps.newHashMap();
+ }
+
+ return jobIds.stream()
+ .collect(
+ Collectors.toMap(
+ jobId -> jobId,
+ jobId ->
+ SessionUtils.getWithoutCommit(
+ JobMetaMapper.class, jobMetaMapper ->
jobMetaMapper.selectJobById(jobId))));
+ }
+
+ private static Map<Long, String> getJobTemplateObjectsFullName(List<Long>
jobTemplateIds) {
+ if (jobTemplateIds == null || jobTemplateIds.isEmpty()) {
+ return Maps.newHashMap();
+ }
+
+ return jobTemplateIds.stream()
+ .collect(
+ Collectors.toMap(
+ jobTemplateId -> jobTemplateId,
+ jobTemplateId ->
+ SessionUtils.getWithoutCommit(
+ JobTemplateMetaMapper.class,
+ jobTemplateMetaMapper ->
+
jobTemplateMetaMapper.selectJobTemplateById(jobTemplateId))));
+ }
+
private static Map<Long, String> getTagObjectsFullName(List<Long> tagIds) {
if (tagIds == null || tagIds.isEmpty()) {
- return Map.of();
+ return Maps.newHashMap();
Review Comment:
Changed from `Map.of()` to `Maps.newHashMap()` for consistency with other
methods in the same file. This is good for maintainability, but note that
`Maps.newHashMap()` returns a mutable map while `Map.of()` returns immutable.
Ensure the caller doesn't rely on immutability.
```suggestion
return Map.of();
```
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/JobMetaBaseSQLProvider.java:
##########
@@ -181,4 +181,19 @@ public String deleteJobMetasByLegacyTimeline(
+ JobMetaMapper.TABLE_NAME
+ " WHERE deleted_at < #{legacyTimeline} AND deleted_at > 0 LIMIT
#{limit}";
}
+
+ public String selectJobById(Long jobId) {
+ return "SELECT jrm.job_run_id AS jobRunId, jtm.job_template_name AS
jobTemplateName,"
+ + " jrm.metalake_id AS metalakeId, jrm.job_execution_id AS
jobExecutionId,"
+ + " jrm.job_run_status AS jobRunStatus, jrm.job_finished_at AS
jobFinishedAt,"
+ + " jrm.audit_info AS auditInfo,"
+ + " jrm.current_version AS currentVersion, jrm.last_version AS
lastVersion,"
+ + " jrm.deleted_at AS deletedAt"
+ + " FROM "
+ + JobMetaMapper.TABLE_NAME
+ + " jrm JOIN "
+ + JobTemplateMetaMapper.TABLE_NAME
+ + " jtm ON jrm.job_template_id = jtm.job_template_id"
+ + " WHERE jrm.job_run_id = #{jobId} AND jrm.deleted_at = 0 AND
jtm.deleted_at = 0";
Review Comment:
The SQL query joins JobMetaMapper and JobTemplateMetaMapper tables, but the
method returns a String (job template name) based on the mapper signature. This
seems inconsistent with the query structure which selects multiple fields.
Consider documenting what the expected return type should be or verify the
mapper method signature matches the query intent.
##########
core/src/main/java/org/apache/gravitino/GravitinoEnv.java:
##########
@@ -603,15 +604,16 @@ private void initGravitinoServerComponents() {
this.auxServiceManager.serviceInit(config);
// Create and initialize Tag related modules
- TagEventDispatcher tagEventDispatcher =
- new TagEventDispatcher(eventBus, new TagManager(idGenerator,
entityStore));
- this.tagDispatcher = new TagHookDispatcher(tagEventDispatcher);
+ this.tagDispatcher =
+ new TagEventDispatcher(
+ eventBus, new TagHookDispatcher(new TagManager(idGenerator,
entityStore)));
Review Comment:
The initialization order was changed - TagHookDispatcher now wraps
TagManager instead of TagEventDispatcher wrapping TagHookDispatcher. Ensure
this change is intentional and doesn't break the event dispatching flow or hook
execution order.
```suggestion
TagManager tagManager = new TagManager(idGenerator, entityStore);
TagHookDispatcher tagHookDispatcher = new TagHookDispatcher(tagManager);
this.tagDispatcher = new TagEventDispatcher(eventBus, tagHookDispatcher);
```
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/JobTemplateMetaBaseSQLProvider.java:
##########
@@ -141,4 +141,24 @@ public String updateJobTemplateMeta(
+ " AND last_version = #{oldJobTemplateMeta.lastVersion}"
+ " AND deleted_at = 0";
}
+
+ public String selectJobTemplateIdByMetalakeAndName(
+ @Param("metalakeId") Long metalakeId, @Param("jobTemplateName") String
jobTemplateName) {
+ return "SELECT job_template_id FROM "
+ + JobTemplateMetaMapper.TABLE_NAME
+ + " WHERE deleted_at = 0 AND metalake_id = #{metalakeId} AND
job_template_name = #{jobTemplateName}";
+ }
+
+ public String selectJobTemplateById(@Param("jobTemplateId") Long
jobTemplateId) {
+ return "SELECT jtm.job_template_id AS jobTemplateId, jtm.job_template_name
AS jobTemplateName,"
+ + " jtm.metalake_id AS metalakeId, jtm.job_template_comment AS
jobTemplateComment,"
+ + " jtm.job_template_content AS jobTemplateContent, jtm.audit_info AS
auditInfo,"
+ + " jtm.current_version AS currentVersion, jtm.last_version AS
lastVersion,"
+ + " jtm.deleted_at AS deletedAt"
+ + " FROM "
+ + JobTemplateMetaMapper.TABLE_NAME
+ + " jtm"
+ + " WHERE jtm.job_template_id = #{jobTemplateId}"
+ + " AND jtm.deleted_at = 0";
Review Comment:
Similar to comment 5, this query selects multiple fields but the
corresponding mapper method returns a String. The mismatch between the query
structure and return type should be clarified or corrected.
--
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]
