jerryshao commented on code in PR #13425:
URL: https://github.com/apache/gravitino/pull/13425#discussion_r4079879969
##########
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:
Thanks, this turned out to be a real upgrade regression rather than an edge
case — see the reply below for details. Went with option 1, walking deeper.
Restricting template names wouldn't help the directories that earlier versions
already created.
##########
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:
You're right, thanks — this is an upgrade regression, not just the
pre-existing rename leak. The legacy layout put the template name into the path
as is, so a template named `team/etl` staged its jobs at
`<stagingDir>/<metalake>/team/etl/job-<id>`, and the old cleanup rebuilt
exactly that path.
Fixed in f804c292c: the legacy lookup now descends below a template's first
path element instead of checking a single level, so a nested legacy directory
is found by job id whether or not the template was renamed. Another job's
staging directory is never descended into, symbolic links are still not
followed, and the depth is bounded at 16 so an unrelated directory tree in the
staging directory isn't walked.
Added two regression tests: an upgraded job with the unchanged template name
`team/etl` whose directory is now deleted by the cleanup, and a lookup test
covering `team/etl`, a deeper `a/b/c/d`, another job's directory, and a path
nested deeper than the bound.
--
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]
