[FLINK-39925][Autoscaler] Skip scaling decisions when vertex IO metrics are incomplete to prevent incorrect scale down#1139
Conversation
|
@Dennis-Mircea, could you please review this PR? This addresses the actual root cause identified in FLINK-39925. |
Dennis-Mircea
left a comment
There was a problem hiding this comment.
Same mentions as in #1136. Before proceeding with the review for this PR please ensure that this fix runs ok on a real cluster (e.g. minikube) and potentially fixes the case described in FLINK-39925.
| "Incomplete IO metrics for vertex {}, skipping scaling decision to avoid incorrect scale down.", | ||
| jobVertexID); | ||
| scalingMetrics.put(ScalingMetric.NUM_RECORDS_IN, Double.NaN); | ||
| scalingMetrics.put(ScalingMetric.NUM_RECORDS_OUT, Double.NaN); |
There was a problem hiding this comment.
As I mentioned in #1136 these metrics cannot be NaN, still you are setting them to NaN and returning. Doesn't seem consistent to me at all.
There was a problem hiding this comment.
Hi @Dennis-Mircea , thanks for the feedback!
I've updated the PR based on your suggestion. The fix is now in ScalingMetricCollector.getJobTopology() instead of ScalingMetrics.java, no NaN anywhere. When any vertex has read-records-complete: false or write-records-complete: false, we now throw NotReadyException directly, which causes the autoscaler to skip the entire collection cycle and retry next interval.
Changes in this update:
- Reverted all changes to IOMetrics.java and ScalingMetrics.java
- Fixed ScalingMetricCollector.java, checks complete flags before building the metrics map
- Added testIncompleteIoMetricsThrowsNotReadyException test using the exact REST API payload reported by Trystan
Only 2 files changed in production code: ScalingMetricCollector.java (+12 lines) and ScalingMetricCollectorTest.java (+1 test).
Regarding cluster testing: I tested on minikube with the fix deployed. The complete: false window is very short in minikube since all pods run on the same node, but the root cause has been confirmed by the reporter (Trystan) on a real cluster, killing the JM and restarting resolved the issue with metrics returning complete: true after restart.
There was a problem hiding this comment.
Hi @Dennis-Mircea, I wanted to follow up on this PR.
As mentioned, the fix has been narrowed down strictly to ScalingMetricCollector.java per your suggestions. Whenever you have some time, please let me know if the current implementation looks good to you.
There was a problem hiding this comment.
Hi @Dennis-Mircea,
Just wanted to gently ping this thread. The fix has been isolated strictly to ScalingMetricCollector.java based on your previous feedback, and cluster testing on minikube confirms the behavior.
Whenever you have a moment, could you please take a look? Let me know if you need any further changes or clarifications before this is ready to merge. Thanks!
…cs are incomplete to prevent incorrect scale down
bc5bb4a to
9abf72e
Compare
What is the purpose of the change
When the JobManager loses connection to TaskManagers (e.g. due to transient network issues on
Kubernetes), the Flink REST API returns
complete: falsefor vertex IO metrics (read-records,write-records) along with zero values. The operator was ignoring thecompleteflag and treatingthese zeros as valid throughput data, causing
TARGET_DATA_RATEto drop to zero and triggeringincorrect scale down even when the job was busy.
This was confirmed by the reporter (FLINK-39925) — killing the JM and letting a new one spin up
resolved the issue, with metrics returning
complete: trueand correct values after restart.Brief change log
ScalingMetricCollector.getJobTopology()to checkisRecordsReadComplete()andisRecordsWrittenComplete()flags before building the vertex metrics mapNotReadyExceptionis thrown, causing theautoscaler to skip the entire collection cycle and retry next interval
IOMetrics.javaorScalingMetrics.javatestIncompleteIoMetricsThrowsNotReadyExceptiontest covering the exact payloadreported in FLINK-39925
Verifying this change
The fix is covered by
ScalingMetricCollectorTestincluding a new testtestIncompleteIoMetricsThrowsNotReadyExceptionwhich uses the exact REST API payloadreported by Trystan (
read-records-complete: false,write-records-complete: falsewithzero values while the job was clearly busy at 33% busy time).
The fix was also deployed on minikube with the operator running successfully.
Does this pull request potentially affect one of the following parts:
CustomResourceDescriptors: noDocumentation
Jira
https://issues.apache.org/jira/browse/FLINK-39925