MartijnVisser commented on code in PR #29460:
URL: https://github.com/apache/flink/pull/29460#discussion_r4251898281
##########
flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/job/rescales/JobRescaleDetailsHandler.java:
##########
@@ -101,20 +103,31 @@ protected JobRescaleDetails handleRequest(
@Override
public Collection<ArchivedJson> archiveJsonWithPath(ExecutionGraphInfo
executionGraphInfo)
throws IOException {
- if (executionGraphInfo.getRescalesStatsSnapshot() == null
- ||
executionGraphInfo.getRescalesStatsSnapshot().getRescaleHistory() == null) {
+ final RescalesStatsSnapshot rescalesStatsSnapshot =
+ executionGraphInfo.getRescalesStatsSnapshot();
+ if (rescalesStatsSnapshot == null ||
rescalesStatsSnapshot.getRescaleHistory() == null) {
return List.of();
}
List<ArchivedJson> archives = new ArrayList<>();
- List<Rescale> rescales =
executionGraphInfo.getRescalesStatsSnapshot().getRescaleHistory();
+ final Map<RescaleIdInfo.RescaleUUID, Rescale> rescalesById = new
LinkedHashMap<>();
Review Comment:
I think adding the latest rescales to `idToRescaleMap` in the constructor is
simpler. `getRescale` loses its fallback and the archive iterates that map, so
both share one set without a new map.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/rest/handler/job/rescales/JobRescaleDetailsHandlerTest.java:
##########
@@ -160,6 +163,80 @@ void testRequestNormalJobRescaleStatisticsDetails()
assertThat(jobRescaleDetails).isEqualTo(fromRescale(rescale, true));
}
+ @Test
+ void testRequestLatestRescaleEvictedFromHistory()
+ throws HandlerRequestException, RestHandlerException {
+ final Rescale latestIgnoredRescale =
+
createTerminatedRescale(TerminatedReason.RESOURCE_REQUIREMENTS_UPDATED);
+ final Rescale retainedCompletedRescale =
+ createTerminatedRescale(TerminatedReason.SUCCEEDED);
+ final ExecutionGraphInfo executionGraphInfo =
+ createExecutionGraphInfo(latestIgnoredRescale,
retainedCompletedRescale);
+ final HandlerRequest<EmptyRequestBody> request =
+ createRequest(
+ executionGraphInfo.getJobId(),
+
latestIgnoredRescale.getRescaleIdInfo().getRescaleUuid());
+
+ assertThat(testInstance.handleRequest(request, executionGraphInfo))
+ .isEqualTo(fromRescale(latestIgnoredRescale, true));
+ }
+
+ @Test
+ void testArchiveIncludesLatestRescaleEvictedFromHistory() throws
IOException {
+ final Rescale latestIgnoredRescale =
+
createTerminatedRescale(TerminatedReason.RESOURCE_REQUIREMENTS_UPDATED);
+ final Rescale retainedCompletedRescale =
+ createTerminatedRescale(TerminatedReason.SUCCEEDED);
+ final ExecutionGraphInfo executionGraphInfo =
+ createExecutionGraphInfo(latestIgnoredRescale,
retainedCompletedRescale);
+
+ final Collection<ArchivedJson> archives =
+ testInstance.archiveJsonWithPath(executionGraphInfo);
+
+ assertThat(archives)
+ .extracting(ArchivedJson::getPath)
+ .containsExactlyInAnyOrder(
+ getDetailsPath(executionGraphInfo,
latestIgnoredRescale),
+ getDetailsPath(executionGraphInfo,
retainedCompletedRescale));
+ }
+
+ private static ExecutionGraphInfo createExecutionGraphInfo(
+ Rescale latestIgnoredRescale, Rescale retainedCompletedRescale) {
+ final RescalesSummary rescalesSummary = new RescalesSummary(1);
+ rescalesSummary.addTerminated(latestIgnoredRescale);
+ rescalesSummary.addTerminated(retainedCompletedRescale);
+ return new ExecutionGraphInfo(
+ new ArchivedExecutionGraphBuilder().build(),
+ List.of(),
+ null,
+ null,
+ new RescalesStatsSnapshot(
+ List.of(retainedCompletedRescale),
Review Comment:
Every rescale here is also a latest one, so the tests stay green with the
history loop in `archiveJsonWithPath` or the history lookup in `getRescale`
removed. Can you add a history-only rescale?
##########
flink-runtime/src/test/java/org/apache/flink/runtime/rest/handler/job/rescales/JobRescaleDetailsHandlerTest.java:
##########
@@ -160,6 +163,80 @@ void testRequestNormalJobRescaleStatisticsDetails()
assertThat(jobRescaleDetails).isEqualTo(fromRescale(rescale, true));
}
+ @Test
+ void testRequestLatestRescaleEvictedFromHistory()
+ throws HandlerRequestException, RestHandlerException {
+ final Rescale latestIgnoredRescale =
+
createTerminatedRescale(TerminatedReason.RESOURCE_REQUIREMENTS_UPDATED);
+ final Rescale retainedCompletedRescale =
+ createTerminatedRescale(TerminatedReason.SUCCEEDED);
+ final ExecutionGraphInfo executionGraphInfo =
+ createExecutionGraphInfo(latestIgnoredRescale,
retainedCompletedRescale);
+ final HandlerRequest<EmptyRequestBody> request =
+ createRequest(
+ executionGraphInfo.getJobId(),
+
latestIgnoredRescale.getRescaleIdInfo().getRescaleUuid());
+
+ assertThat(testInstance.handleRequest(request, executionGraphInfo))
+ .isEqualTo(fromRescale(latestIgnoredRescale, true));
Review Comment:
Please also request an unknown UUID against this snapshot and assert the
404. Without the UUID filter in `getRescale` the suite still passes whenever
`Map.of` iterates the ignored rescale first.
--
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]
