This is an automated email from the ASF dual-hosted git repository. github-actions[bot] pushed a commit to branch cherry-pick-60cd0391-to-branch-1.3 in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit 6c8bb8ad99ec7a90a41b0e651bf24f8f5b978427 Author: Jerry Shao <[email protected]> AuthorDate: Wed Oct 21 10:41:27 2026 +0800 [#14017] fix(core): Return JobTemplateAlreadyExistsException when renaming a job template to an existing name (#14018) ### What changes were proposed in this pull request? - `JobManager.alterJobTemplate` now converts `EntityAlreadyExistsException` into `JobTemplateAlreadyExistsException` instead of a plain `RuntimeException`, and unwraps the new name so the message no longer prints `Optional[...]`. - Declare `JobTemplateAlreadyExistsException` on `SupportsJobs#alterJobTemplate` and `JobOperationDispatcher#alterJobTemplate`. ### Why are the changes needed? Renaming a job template to an existing name returned 500 with a misleading message. It should return 409 so clients can ask the user to pick another name. Fix: #14017 ### Does this PR introduce _any_ user-facing change? Yes. A rename collision now returns HTTP 409 (`JobTemplateAlreadyExistsException`) instead of 500. Java and Python clients already map 409 to `JobTemplateAlreadyExistsException`. ### How was this patch tested? - Added `TestJobManager#testAlterJobTemplateRenameToExistingName`. - Added a 409 case to `TestJobOperations#testAlterJobTemplate`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <[email protected]> # Conflicts: # core/src/test/java/org/apache/gravitino/job/TestJobManager.java # server/src/test/java/org/apache/gravitino/server/web/rest/TestJobOperations.java --- .../org/apache/gravitino/job/SupportsJobs.java | 5 +- .../java/org/apache/gravitino/job/JobManager.java | 22 ++-- .../gravitino/job/JobOperationDispatcher.java | 5 +- .../org/apache/gravitino/job/TestJobManager.java | 123 +++++++++++++++++++++ .../server/web/rest/TestJobOperations.java | 38 +++++++ 5 files changed, 184 insertions(+), 9 deletions(-) diff --git a/api/src/main/java/org/apache/gravitino/job/SupportsJobs.java b/api/src/main/java/org/apache/gravitino/job/SupportsJobs.java index b5ca1698e2..c2e977e29b 100644 --- a/api/src/main/java/org/apache/gravitino/job/SupportsJobs.java +++ b/api/src/main/java/org/apache/gravitino/job/SupportsJobs.java @@ -80,10 +80,13 @@ public interface SupportsJobs { * @param changes the changes to apply to the job template * @return the updated job template after applying the changes * @throws NoSuchJobTemplateException if no job template with the specified name exists + * @throws JobTemplateAlreadyExistsException if the job template is renamed to a name that already + * exists * @throws IllegalArgumentException if any of the changes cannot be applied to the job template */ default JobTemplate alterJobTemplate(String jobTemplateName, JobTemplateChange... changes) - throws NoSuchJobTemplateException, IllegalArgumentException { + throws NoSuchJobTemplateException, JobTemplateAlreadyExistsException, + IllegalArgumentException { throw new UnsupportedOperationException("Not implemented"); } diff --git a/core/src/main/java/org/apache/gravitino/job/JobManager.java b/core/src/main/java/org/apache/gravitino/job/JobManager.java index bd6e8ecc8e..021648f0c4 100644 --- a/core/src/main/java/org/apache/gravitino/job/JobManager.java +++ b/core/src/main/java/org/apache/gravitino/job/JobManager.java @@ -309,7 +309,8 @@ public class JobManager implements JobOperationDispatcher { @Override public JobTemplateEntity alterJobTemplate( String metalake, String jobTemplateName, JobTemplateChange... changes) - throws NoSuchJobTemplateException, IllegalArgumentException { + throws NoSuchJobTemplateException, JobTemplateAlreadyExistsException, + IllegalArgumentException { checkMetalake(NameIdentifierUtil.ofMetalake(metalake), entityStore); Optional<String> newName = @@ -343,12 +344,19 @@ public class JobManager implements JobOperationDispatcher { } 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); + String conflictName = + newName.orElseThrow( + () -> + new IllegalStateException( + "Job template already exists without a rename change", e)); + throw new JobTemplateAlreadyExistsException( + e, + "Failed to rename job template %s to %s under metalake %s, a job template named " + + "%s already exists", + jobTemplateName, + conflictName, + metalake, + conflictName); } }); } diff --git a/core/src/main/java/org/apache/gravitino/job/JobOperationDispatcher.java b/core/src/main/java/org/apache/gravitino/job/JobOperationDispatcher.java index 795ae6aeae..f22114491a 100644 --- a/core/src/main/java/org/apache/gravitino/job/JobOperationDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/job/JobOperationDispatcher.java @@ -81,11 +81,14 @@ public interface JobOperationDispatcher extends Closeable { * @param changes the changes to apply to the job template * @return the updated job template entity after applying the changes * @throws NoSuchJobTemplateException if no job template with the specified name exists + * @throws JobTemplateAlreadyExistsException if the job template is renamed to a name that already + * exists * @throws IllegalArgumentException if any of the changes cannot be applied to the job template. */ JobTemplateEntity alterJobTemplate( String metalake, String jobTemplateName, JobTemplateChange... changes) - throws NoSuchJobTemplateException, IllegalArgumentException; + throws NoSuchJobTemplateException, JobTemplateAlreadyExistsException, + IllegalArgumentException; /** * List all the jobs. If the jobTemplateName is provided, it will list the jobs associated with diff --git a/core/src/test/java/org/apache/gravitino/job/TestJobManager.java b/core/src/test/java/org/apache/gravitino/job/TestJobManager.java index 1d73004281..82efe06c26 100644 --- a/core/src/test/java/org/apache/gravitino/job/TestJobManager.java +++ b/core/src/test/java/org/apache/gravitino/job/TestJobManager.java @@ -175,6 +175,129 @@ public class TestJobManager { } @Test +<<<<<<< HEAD +======= + public void testRegisterJobTemplateReportsConcurrentMetalakeDeletion() throws IOException { + JobTemplateEntity template = newShellJobTemplateEntity("shell_job", "A shell job template"); + NoSuchEntityException missing = new NoSuchEntityException("Metalake was deleted"); + doThrow(missing).when(entityStore).put(template, false); + + NoSuchMetalakeException failure = + Assertions.assertThrows( + NoSuchMetalakeException.class, + () -> jobManager.registerJobTemplate(metalake, template)); + Assertions.assertSame(missing, failure.getCause()); + } + + @Test + public void testRunJobReportsParentDisappearingDuringRegistration() throws Exception { + JobTemplateEntity template = newShellJobTemplateEntity("shell_job", "A shell job template"); + doReturn(template).when(jobManager).getJobTemplate(metalake, template.name()); + for (Entity.EntityType parent : + List.of(Entity.EntityType.METALAKE, Entity.EntityType.JOB_TEMPLATE)) { + Mockito.reset(entityStore, jobExecutor); + String executionId = "submitted_" + parent.name(); + when(jobExecutor.submitJob(any(), any())).thenReturn(executionId); + NoSuchEntityException missing = new NoSuchEntityException("Parent was deleted: %s", parent); + doThrow(missing).when(entityStore).put(any(JobEntity.class), eq(false)); + + NoSuchJobTemplateException failure = + Assertions.assertThrows( + NoSuchJobTemplateException.class, + () -> jobManager.runJob(metalake, template.name(), Collections.emptyMap())); + Assertions.assertSame(missing, failure.getCause()); + verify(jobExecutor, times(1)).submitJob(any(), any()); + verify(jobExecutor, never()).cancelJob(any()); + verify(entityStore, times(1)).put(any(JobEntity.class), eq(false)); + } + } + + @Test + public void testRunJobCancelsExecutionWhenRegistrationConflicts() throws Exception { + JobTemplateEntity template = newShellJobTemplateEntity("shell_job", "A shell job template"); + doReturn(template).when(jobManager).getJobTemplate(metalake, template.name()); + Mockito.reset(entityStore, jobExecutor); + when(jobExecutor.submitJob(any(), any())).thenReturn("submitted_conflict"); + OptimisticLockException conflict = new OptimisticLockException("lock wait timeout"); + doThrow(conflict).when(entityStore).put(any(JobEntity.class), eq(false)); + + // Cancellation cannot guarantee that the execution has not already performed work. + RuntimeException failure = + Assertions.assertThrows( + RuntimeException.class, + () -> jobManager.runJob(metalake, template.name(), Collections.emptyMap())); + Assertions.assertFalse(failure instanceof OptimisticLockException); + Assertions.assertSame(conflict, failure.getCause()); + Assertions.assertTrue(failure.getMessage().contains("submitted_conflict")); + Assertions.assertTrue(failure.getMessage().contains("Check the execution before retrying")); + verify(jobExecutor, times(1)).cancelJob("submitted_conflict"); + + // A failed cancellation is kept on the cause for diagnosis and is not a retryable conflict. + Mockito.reset(jobExecutor); + when(jobExecutor.submitJob(any(), any())).thenReturn("submitted_conflict_2"); + RuntimeException cancelFailure = new RuntimeException("executor unavailable"); + doThrow(cancelFailure).when(jobExecutor).cancelJob("submitted_conflict_2"); + RuntimeException second = + Assertions.assertThrows( + RuntimeException.class, + () -> jobManager.runJob(metalake, template.name(), Collections.emptyMap())); + Assertions.assertFalse(second instanceof OptimisticLockException); + Assertions.assertSame(conflict, second.getCause()); + Assertions.assertTrue(second.getMessage().contains("submitted_conflict_2")); + Assertions.assertTrue(List.of(second.getCause().getSuppressed()).contains(cancelFailure)); + } + + @Test + public void testAlterJobTemplateDistinguishesMissingFromConflict() throws IOException { + NoSuchEntityException missing = new NoSuchEntityException("Template was deleted"); + doThrow(missing).when(entityStore).update(any(), any(), any(), any()); + NoSuchJobTemplateException failure = + Assertions.assertThrows( + NoSuchJobTemplateException.class, + () -> jobManager.alterJobTemplate(metalake, "shell_job")); + Assertions.assertEquals( + "Job template with name shell_job under metalake " + metalake + " does not exist", + failure.getMessage()); + + OptimisticLockException conflict = new OptimisticLockException("Template was modified"); + doThrow(conflict).when(entityStore).update(any(), any(), any(), any()); + Assertions.assertSame( + conflict, + Assertions.assertThrows( + OptimisticLockException.class, + () -> jobManager.alterJobTemplate(metalake, "shell_job"))); + } + + @Test + public void testAlterJobTemplateRenameToExistingName() throws IOException { + EntityAlreadyExistsException exists = new EntityAlreadyExistsException("Name already exists"); + doThrow(exists).when(entityStore).update(any(), any(), any(), any()); + + JobTemplateAlreadyExistsException failure = + Assertions.assertThrows( + JobTemplateAlreadyExistsException.class, + () -> + jobManager.alterJobTemplate( + metalake, "shell_job", JobTemplateChange.rename("existing_job"))); + Assertions.assertSame(exists, failure.getCause()); + Assertions.assertEquals( + "Failed to rename job template shell_job to existing_job under metalake " + + metalake + + ", a job template named existing_job already exists", + failure.getMessage()); + + // Without a rename change, a conflict is an invariant violation, not a name conflict. + IllegalStateException unexpected = + Assertions.assertThrows( + IllegalStateException.class, + () -> + jobManager.alterJobTemplate( + metalake, "shell_job", JobTemplateChange.updateComment("new comment"))); + Assertions.assertSame(exists, unexpected.getCause()); + } + + @Test +>>>>>>> 60cd03915 ([#14017] fix(core): Return JobTemplateAlreadyExistsException when renaming a job template to an existing name (#14018)) public void testListJobTemplates() throws IOException { mockedMetalake .when(() -> MetalakeManager.checkMetalake(metalakeIdent, entityStore)) diff --git a/server/src/test/java/org/apache/gravitino/server/web/rest/TestJobOperations.java b/server/src/test/java/org/apache/gravitino/server/web/rest/TestJobOperations.java index da79ca5bb7..feb243862b 100644 --- a/server/src/test/java/org/apache/gravitino/server/web/rest/TestJobOperations.java +++ b/server/src/test/java/org/apache/gravitino/server/web/rest/TestJobOperations.java @@ -648,6 +648,44 @@ public class TestJobOperations extends JerseyTest { Assertions.assertEquals(ErrorConstants.NOT_FOUND_CODE, errorResp4.getCode()); Assertions.assertEquals(NoSuchJobTemplateException.class.getSimpleName(), errorResp4.getType()); +<<<<<<< HEAD +======= + // 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)); + + Assertions.assertEquals(Response.Status.CONFLICT.getStatusCode(), respExists.getStatus()); + ErrorResponse errorRespExists = respExists.readEntity(ErrorResponse.class); + Assertions.assertEquals(ErrorConstants.ALREADY_EXISTS_CODE, errorRespExists.getCode()); + Assertions.assertEquals( + JobTemplateAlreadyExistsException.class.getSimpleName(), errorRespExists.getType()); + + // Test throw UnsupportedOperationException + doThrow(new UnsupportedOperationException("mock error")) + .when(jobOperationDispatcher) + .alterJobTemplate(any(), any(), any()); + + Response resp5b = + target(jobTemplatePath()) + .path(templateName) + .request(APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .put(Entity.entity(req, APPLICATION_JSON_TYPE)); + + Assertions.assertEquals(Response.Status.NOT_IMPLEMENTED.getStatusCode(), resp5b.getStatus()); + ErrorResponse errorResp4b = resp5b.readEntity(ErrorResponse.class); + Assertions.assertEquals( + UnsupportedOperationException.class.getSimpleName(), errorResp4b.getType()); + +>>>>>>> 60cd03915 ([#14017] fix(core): Return JobTemplateAlreadyExistsException when renaming a job template to an existing name (#14018)) // Test throw RuntimeException doThrow(new RuntimeException("mock error")) .when(jobOperationDispatcher)
