yuqi1129 commented on code in PR #11940: URL: https://github.com/apache/gravitino/pull/11940#discussion_r3541937735
########## 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: Good point — removed the wrapping; `ensureDirectory` now lets `Files.createDirectories` propagate its original `IOException` (including `FileAlreadyExistsException`), and the Javadoc documents both. Fixed in 3190c6f. ########## 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: Agreed — the test now constructs its own `JobManager` via the `@VisibleForTesting` constructor with a mocked `IdGenerator`, shuts down its schedulers like `setUp` does, and closes it in a `finally` block. No reflective write to the final field anymore. Verified the test still fails against the unfixed `JobManager`. Fixed in 3190c6f. -- 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]
