gandherva commented on code in PR #29460:
URL: https://github.com/apache/flink/pull/29460#discussion_r4250138457
##########
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 wanted to offer this suggestion to avoid creation of LinkedHashMap.
Instead of building a `LinkedHashMap` of the whole history just to dedupe it
against at most 3 latest rescales (one per `TerminalState`), we could archive
the history as is and only append the latest rescales that aren't already in
it.
`RescalesStatsSnapshot` already maintains `idToRescaleMap` (rescale ID to
`Rescale`) for the history. We can't use `getRescale()` for this check, because
this PR makes it fall back to `latestRescales` and it is no longer just
history. A small history only method could be created to serve this.
Some sample code change and there are other ways to do it, upto you.
In RescalesStatsSnapshot
```java
public boolean isInHistory(AbstractID rescaleId) {
return idToRescaleMap.containsKey(rescaleId);
}
```
In JobRescaleDetailsHandler
```java
for (Rescale rescale : rescalesStatsSnapshot.getRescaleHistory()) {
archives.add(createArchivedJson(commonPathPrefix, rescale));
}
for (TerminalState terminalState : TerminalState.values()) {
final Rescale latestRescale =
rescalesStatsSnapshot.getLatestRescale(terminalState);
if (latestRescale != null
&& !rescalesStatsSnapshot.isInHistory(
latestRescale.getRescaleIdInfo().getRescaleUuid())) {
archives.add(createArchivedJson(commonPathPrefix, latestRescale));
}
}
return archives;
```
We would need a createArchivedJson helper method.
This option has no new map creation.
--
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]
