Copilot commented on code in PR #29008:
URL: https://github.com/apache/flink/pull/29008#discussion_r4302350218
##########
flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/history/HistoryServerArchiveFetcher.java:
##########
@@ -669,6 +731,29 @@ void lazyFetchArchiveProactively(String jobId, @Nullable
Path archivePath) throw
}
}
+ /** Marks the job as recently viewed and evicts the least recently viewed
ones locally. */
+ void markRecentlyViewed(String jobId) {
+ List<ArchiveEvent> events = new ArrayList<>();
+ synchronized (mostRecentlyViewedJobs) {
+ mostRecentlyViewedJobs.put(jobId, Boolean.TRUE);
+ Iterator<String> eldest =
mostRecentlyViewedJobs.keySet().iterator();
+ while (mostRecentlyViewedJobs.size() >
numCachedMostRecentlyViewedJobs) {
+ String evictedId = eldest.next();
+ eldest.remove();
+ // archives still inside the cached window are managed by the
scan
+ if (cachedArchivesPerRefreshDirectory.values().stream()
+ .noneMatch(ids -> ids.contains(evictedId))) {
+ cleanUpLazyFetchTask(evictedId);
+ events.add(deleteJobFiles(evictedId));
Review Comment:
In LAZY mode, this also evicts application-owned jobs, even when
`cached-retained-jobs` is unset. The handler marks every requested job, but
application jobs are tracked by `HistoryServerApplicationArchiveFetcher`, not
this fetcher's `cachedArchivesPerRefreshDirectory`. With the default viewed-job
limit of 1, viewing application jobs A then B deletes A's local files and
shared metadata. Revisiting A cannot reload it: proactive fetching searches
only `<refreshDir>/<jobId>`, while A is nested under an application directory,
and the application fetcher skips the already-cached application. Exclude
application-owned jobs from this eviction policy and add a regression test that
revisits A after viewing B with default cache settings.
##########
flink-runtime-web/src/test/java/org/apache/flink/runtime/webmonitor/history/HistoryServerArchiveFetcherTest.java:
##########
@@ -420,6 +455,102 @@ void testScanArchivesWithoutFetch() throws Exception {
assertThat(archiveStorage.exists("overviews/" + jobId +
".json")).isFalse();
}
+ @TestTemplate
+ void testArchivesBeyondCachedLimitAreOnlyCachedLocallyWithinLimit() throws
Exception {
+ JobID newest = JobID.generate();
+ JobID older = JobID.generate();
+ Path olderArchive = createArchive(older, 1000);
+ createArchive(newest, 2000);
+
+ fetcher = createArchiveFetcher(RETAIN_ALL, 1, 1);
+ fetcher.fetchArchives(EAGER);
+
+ assertThat(isCachedLocally(newest)).isTrue();
+ assertThat(isCachedLocally(older)).isFalse();
+ assertThat(olderArchive.getFileSystem().exists(olderArchive)).isTrue();
+ }
+
+ @TestTemplate
+ void testCachedArchivePushedOutOfLimitIsEvictedLocallyOnly() throws
Exception {
+ JobID first = JobID.generate();
+ JobID second = JobID.generate();
+ Path firstArchive = createArchive(first, 1000);
+
+ fetcher = createArchiveFetcher(RETAIN_ALL, 1, 1);
+ fetcher.fetchArchives(EAGER);
+ assertThat(isCachedLocally(first)).isTrue();
+
+ createArchive(second, 2000);
+ archiveEvents.clear();
+ fetcher.fetchArchives(EAGER);
+
+ assertThat(isCachedLocally(first)).isFalse();
+ assertThat(isCachedLocally(second)).isTrue();
+ assertThat(firstArchive.getFileSystem().exists(firstArchive)).isTrue();
+ assertThat(archiveEvents)
+ .anyMatch(
+ e ->
+ e.getId().equals(first.toString())
+ && e.getType()
+ ==
HistoryServerArchiveFetcher.ArchiveEventType
+ .DELETED);
+ }
+
+ @TestTemplate
+ void testViewedArchiveBeyondLimitSurvivesNextScan() throws Exception {
+ JobID newest = JobID.generate();
+ JobID older = JobID.generate();
+ Path olderArchive = createArchive(older, 1000);
+ createArchive(newest, 2000);
+
+ fetcher = createArchiveFetcher(RETAIN_ALL, 1, 1);
+ fetcher.fetchArchives(EAGER);
+ view(fetcher, older, olderArchive);
+ assertThat(isCachedLocally(older)).isTrue();
+
+ archiveEvents.clear();
+ fetcher.fetchArchives(EAGER);
+
+ assertThat(archiveEvents).isEmpty();
+ assertThat(isCachedLocally(older)).isTrue();
Review Comment:
Both archives exist before the first scan, so `older` never enters
`cachedArchivesPerRefreshDirectory`; proactive fetching does not add it either.
The next scan therefore never considers it for local eviction, and this test
passes even if the recently-viewed exclusion is removed. Cache and mark `older`
as viewed before creating `newest`, then scan and verify that both remain
cached and no deletion event is emitted for `older`. This exercises the scan's
new protection.
--
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]
