Copilot commented on code in PR #14018:
URL: https://github.com/apache/gravitino/pull/14018#discussion_r4323655162
##########
core/src/main/java/org/apache/gravitino/job/JobManager.java:
##########
@@ -371,12 +372,14 @@ public JobTemplateEntity alterJobTemplate(
} catch (EntityAlreadyExistsException e) {
// If the EntityAlreadyExistsException is thrown, it means the new
name already exists.
// So there should be a rename change, and the new name should be
present.
- throw new RuntimeException(
- String.format(
- "Failed to rename job template from %s to %s under
metalake %s, the new name "
- + "already exists",
- jobTemplateName, newName, metalake),
- e);
+ throw new JobTemplateAlreadyExistsException(
+ e,
+ "Failed to rename job template %s to %s under metalake %s, a
job template named "
+ + "%s already exists",
+ jobTemplateName,
+ newName.orElse(jobTemplateName),
+ metalake,
+ newName.orElse(jobTemplateName));
Review Comment:
`newName.orElse(jobTemplateName)` is used twice, which both duplicates logic
and can silently produce a misleading message if `newName` is unexpectedly
empty in this catch block. Since the comment above states the new name “should
be present”, unwrap once (e.g., resolve to a local `String`) and fail fast if
absent (e.g., `orElseThrow`) so an invariant violation doesn’t masquerade as a
legitimate “already exists” conflict.
##########
server/src/test/java/org/apache/gravitino/server/web/rest/TestJobOperations.java:
##########
@@ -674,6 +674,24 @@ public void testAlterJobTemplate() {
Assertions.assertEquals(ErrorConstants.NOT_FOUND_CODE,
errorResp4.getCode());
Assertions.assertEquals(NoSuchJobTemplateException.class.getSimpleName(),
errorResp4.getType());
+ // Test throw JobTemplateAlreadyExistsException
+ doThrow(new JobTemplateAlreadyExistsException("mock error"))
+ .when(jobOperationDispatcher)
+ .alterJobTemplate(any(), any(), any());
+
+ Response respExists =
+ target(jobTemplatePath())
+ .path(templateName)
+ .request(APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .put(Entity.entity(req, APPLICATION_JSON_TYPE));
Review Comment:
`Response` is `AutoCloseable`; the newly added `respExists` should be closed
(e.g., via try-with-resources) to avoid leaking underlying HTTP resources
during the test run. Align this new block with whatever resource-management
pattern the rest of the test method/file uses.
--
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]
