danmagyar commented on a change in pull request #12636:
URL: https://github.com/apache/flink/pull/12636#discussion_r441394649
##########
File path:
flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/history/HistoryServerArchiveFetcher.java
##########
@@ -295,6 +315,19 @@ public void run() {
}
}
+ private List<ArchiveEvent> cleanupJobsBeyondSizeLimit(Set<Path>
jobArchivesToRemove) {
+ Set<String> jobIdsToRemoveFromOverview = new
HashSet<>();
+ for (Path archive : jobArchivesToRemove) {
+
jobIdsToRemoveFromOverview.add(archive.getName());
+ try {
+ archive.getFileSystem().delete(archive,
false);
+ } catch (IOException ioe) {
+ LOG.error("Error while deleting old
archive " + archive, ioe);
+ }
+ }
+ return cleanupExpiredJobs(jobIdsToRemoveFromOverview);
Review comment:
Thanks for pointing this out @zentol, I didn't pay attention to this
redundant operation as it is only present if
`historyserver.archive.clean-expired-jobs` is explicitly set to `true` (`false`
is the default) and I didn't cover this configuration in the tests. That is
jobs in `jobsToRemove` are not processed by default as per
[L302](https://github.com/apache/flink/pull/12636/files/c91fac7a9ceaae8605e7146cdeffa00e2a15e187#diff-ed2f12d42964612046a16b3ef5aa7ffeR302).
Yet I agree, the currently non-default case should be taken into account
too, I'd propose simply not skipping the `jobsToRemove.remove(jobID);` if the
job is added to `archivesBeyondSizeLimit`. I'll shortly update the PR
accordingly. What do you think of this?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
