lasdf1234 commented on code in PR #13425:
URL: https://github.com/apache/gravitino/pull/13425#discussion_r4079724715
##########
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:
Nit / edge case: `findLegacyJobStagingDirs` only looks one level under each
metalake directory (`<metalake>/<child>/job-<id>`).
The pre-upgrade layout used the raw template name in the path
(`String.format(..., metalake, jobTemplateName, jobId)`). If a template name
ever contains `/` (e.g. `team/etl`), the directory would be nested deeper
(`<metalake>/team/etl/job-<id>`), and this lookup would miss it — so those
legacy staging dirs could still leak on cleanup / template delete.
There is currently no validation that forbids `/` in job template names
(only non-blank + no `builtin-` prefix). In practice this is unlikely via the
REST path `.../templates/{name}`, but worth either:
1. walking deeper (still skipping symlinks / hidden entries), or
2. documenting / enforcing that template names must not contain `/`.
Not blocking for the common case.
--
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]
