jerryshao commented on code in PR #14018:
URL: https://github.com/apache/gravitino/pull/14018#discussion_r4323755919
##########
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:
Unwrapped the new name once into `conflictName` via `orElseThrow`, so a
conflict without a rename change now surfaces as an `IllegalStateException`
instead of a misleading "already exists" message. Added a test for that path.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
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:
No change here: the rest of this file does not close `Response` objects
either, and per JAX-RS `readEntity` closes the response once the entity is
consumed (it is not an `InputStream`), so nothing leaks.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
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]
