Copilot commented on code in PR #11940:
URL: https://github.com/apache/gravitino/pull/11940#discussion_r3541830945
##########
core/src/test/java/org/apache/gravitino/job/TestJobManager.java:
##########
@@ -516,6 +516,32 @@ public void testRunJob() throws IOException {
() -> jobManager.runJob(metalake, "shell_job",
Collections.emptyMap()));
}
+ @Test
+ public void testRunJobSucceedsWhenStagingDirectoryAlreadyExists() throws
Exception {
+ mockedMetalake
+ .when(() -> MetalakeManager.checkMetalake(metalakeIdent, entityStore))
+ .thenAnswer(a -> null);
+
+ JobTemplateEntity shellJobTemplate =
+ newShellJobTemplateEntity("shell_job", "A shell job template");
+ when(jobManager.getJobTemplate(metalake,
shellJobTemplate.name())).thenReturn(shellJobTemplate);
+ when(jobExecutor.submitJob(any())).thenReturn("job_execution_id_for_test");
+ doNothing().when(entityStore).put(any(JobEntity.class), anyBoolean());
+
+ // Use a fixed job ID so that both runs resolve to the same staging
directory.
+ IdGenerator fixedIdGenerator = Mockito.mock(IdGenerator.class);
+ when(fixedIdGenerator.nextId()).thenReturn(12345L);
+ FieldUtils.writeField(jobManager, "idGenerator", fixedIdGenerator, true);
+
+ JobEntity first = jobManager.runJob(metalake, "shell_job",
Collections.emptyMap());
+ Assertions.assertEquals(12345L, first.id());
+
+ // The staging directory for job 12345 exists now; running the job again
must not fail on
+ // directory creation.
+ JobEntity second = jobManager.runJob(metalake, "shell_job",
Collections.emptyMap());
+ Assertions.assertEquals(12345L, second.id());
Review Comment:
This test mutates `JobManager`'s `private final IdGenerator idGenerator` via
reflection (`FieldUtils.writeField`). Updating `final` instance fields
reflectively is not reliably supported across JVMs/JIT optimizations and can
make the test flaky. Prefer constructing a `JobManager` with the fixed
`IdGenerator` using the existing visible-for-testing constructor, and shut down
its schedulers like the main test setup does.
##########
common/src/main/java/org/apache/gravitino/utils/DirectoryUtils.java:
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.utils;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+
+/** Utilities for working with local directories. */
+public class DirectoryUtils {
+
+ private DirectoryUtils() {}
+
+ /**
+ * Ensures that the given directory exists, creating it and any missing
parent directories if
+ * necessary.
+ *
+ * <p>Unlike {@code File#exists()} followed by {@code File#mkdirs()}, this
method is safe against
+ * concurrent creation of the same directory: it succeeds if the directory
already exists or is
+ * created concurrently by another thread or process.
+ *
+ * @param dir the directory to create
+ * @throws IOException if the directory cannot be created, or if the path
exists but is not a
+ * directory
+ */
+ public static void ensureDirectory(File dir) throws IOException {
+ try {
+ Files.createDirectories(dir.toPath());
+ } catch (IOException e) {
+ throw new IOException(
+ String.format("Failed to create directory %s",
dir.getAbsolutePath()), e);
+ }
+ }
Review Comment:
`ensureDirectory` catches and wraps all `IOException`s into a new generic
`IOException`, which (a) hides the original exception subtype (e.g.,
`FileAlreadyExistsException` when the path exists as a file) and (b) tends to
produce double-wrapped, repetitive messages at callers that already add
context. Let `Files.createDirectories` throw its original `IOException` so
callers can preserve details while still getting a useful stacktrace.
--
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]
