lasdf1234 commented on code in PR #14016:
URL: https://github.com/apache/gravitino/pull/14016#discussion_r4332868980
##########
core/src/main/java/org/apache/gravitino/job/JobManager.java:
##########
@@ -1363,6 +1349,53 @@ private Optional<JobEntity> updateJobEntity(
}
}
+ private void cleanUpStagingDirs(String metalake) {
+ long now = System.currentTimeMillis();
+ List<JobEntity> expiredJobs = new ArrayList<>();
+ for (JobEntity job : listJobs(metalake, Optional.empty())) {
+ if (isFinishedStatus(job.status())) {
+ if (job.finishedAt() > 0 && job.finishedAt() +
jobStagingDirKeepTimeInMs < now) {
Review Comment:
**[Low] `finishedAt()` null unboxing can abort the rest of this metalake's
cleanup**
`JobEntity.finishedAt()` returns `Long` and may be null. `job.finishedAt() >
0` unboxes it, so a finished job with a null `finishedAt` throws
`NullPointerException` here.
That NPE escapes the `for` loop (it is outside the per-job try below), lands
in `forEachInUseMetalake`'s metalake-level handler, and skips the
`expiredJobs.forEach(...)` delete batch for jobs already collected earlier in
this run — the same isolation hole this PR is closing elsewhere.
Pre-existing on `main`, but it undercuts the new guards. The file already
has `timeInMs(@Nullable Long)` for this:
```java
long finishedAt = timeInMs(job.finishedAt());
if (finishedAt > 0 && finishedAt + jobStagingDirKeepTimeInMs < now) {
expiredJobs.add(job);
}
```
--
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]
