afedulov commented on code in PR #759:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/759#discussion_r1470917165
##########
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingMetricCollector.java:
##########
@@ -181,9 +186,12 @@ private static Instant getWindowFullTime(
}
@VisibleForTesting
- protected Instant getJobUpdateTs(JobDetailsInfo jobDetailsInfo) {
- return Instant.ofEpochMilli(
-
jobDetailsInfo.getTimestamps().values().stream().max(Long::compare).get());
+ protected Instant getJobSwitchToRunningTs(JobDetailsInfo jobDetailsInfo) {
+ final Map<JobStatus, Long> timestamps = jobDetailsInfo.getTimestamps();
+
+ final Long runningTs = timestamps.get(JobStatus.RUNNING);
+ checkState(runningTs != null, "Unable to find when the job was
switched to RUNNING.");
Review Comment:
>We can add this invariant here but it would only defend against a Byzantine
behavior.
I don't quite agree - I find it it a bit one-sided to look at public classes
and methods only from the perspective of the implicit context in which they are
**currently** called.
I believe the check is appropriate - nothing in the contract or comments of
`updateMetrics` states the scope in which it is supposed to be executed. The
class is also open for extension - someone can later override methods partially
without being clear about the implicit assumptions. An alternative is to seal
everything and make this class only available to `JobAutoscalerImpl`.
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
