yuqi1129 commented on code in PR #13425:
URL: https://github.com/apache/gravitino/pull/13425#discussion_r4079761303
##########
core/src/main/java/org/apache/gravitino/job/JobManager.java:
##########
@@ -1108,6 +1096,72 @@ JobTemplateEntity updateJobTemplateEntity(
.build();
}
+ @VisibleForTesting
+ File jobStagingDir(long jobId) {
+ return new File(new File(stagingDir, JOB_RUNS_DIR_NAME),
JobHandle.JOB_ID_PREFIX + jobId);
+ }
+
+ /**
+ * Finds the staging directory of a job submitted by an earlier Gravitino
version, which used the
+ * {@code <stagingDir>/<metalake>/<template>/job-<id>} layout. It's looked
up by the job id rather
+ * than rebuilt from the current names, because the template or metalake may
have been renamed
+ * since the job ran. The job id is unique, so there is at most one match in
practice. Called only
+ * for a job without a directory in the current layout, which after an
upgrade are the jobs of the
+ * earlier version until they expire.
+ */
+ @VisibleForTesting
+ List<File> findLegacyJobStagingDirs(long jobId) throws IOException {
+ String jobDirName = JobHandle.JOB_ID_PREFIX + jobId;
+ List<File> legacyJobStagingDirs = new ArrayList<>();
+ try (DirectoryStream<Path> metalakeDirs =
Files.newDirectoryStream(stagingDir.toPath())) {
+ for (Path metalakeDir : metalakeDirs) {
+ String name = metalakeDir.getFileName().toString();
+ // Metalake names can't start with '.', so hidden entries, e.g. the
job output index
+ // directory, are not metalake directories. Symbolic links are never
followed, so nothing
+ // outside the staging directory can be deleted.
+ if (name.equals(JOB_RUNS_DIR_NAME)
+ || name.startsWith(".")
+ || !Files.isDirectory(metalakeDir, LinkOption.NOFOLLOW_LINKS)) {
+ continue;
+ }
+
+ // An unreadable directory, e.g. "lost+found" when the staging
directory is the root of a
+ // file system, must not prevent finding the job under the other
directories.
+ try (DirectoryStream<Path> templateDirs =
Files.newDirectoryStream(metalakeDir)) {
+ for (Path templateDir : templateDirs) {
+ Path jobDir = templateDir.resolve(jobDirName);
Review Comment:
This is an upgrade regression, not just a pre-existing rename leak. Before
this PR, a job submitted from `team/etl` was created at
`<stagingDir>/<metalake>/team/etl/job-<id>`, and cleanup rebuilt that same path
from the unchanged template name, so it could delete it. The pre-upgrade
`TestJobManagerMultiNode.testGetJobOutputOfTemplateNamedWithSpecialCharacters`
already runs a job with `team/etl` successfully.
After an upgrade, even with no rename, `findLegacyJobStagingDirs()` checks
only `<metalake>/<one child>/job-<id>` and misses that existing directory. I
changed the legacy cleanup test locally to place the job under `team/etl`:
`entityStore.delete(JOB)` was called, but the staging directory still existed,
so the test failed. The test-only change has been reverted.
I think the compatibility lookup needs to cover nested legacy template paths
without following symlinks, with a regression test for an upgraded job whose
template name contains `/`. Restricting new template names would not clean
directories already created by older versions.
--
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]
