1996fanrui commented on code in PR #677:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/677#discussion_r1360079090
##########
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingMetricCollector.java:
##########
@@ -441,15 +442,21 @@ protected Collection<AggregatedMetric>
queryAggregatedMetricNames(
protected abstract Map<JobVertexID, Map<FlinkMetric, AggregatedMetric>>
queryAllAggregatedMetrics(
- AbstractFlinkResource<?, ?> cr,
- FlinkService flinkService,
- Configuration conf,
+ Context ctx,
Map<JobVertexID, Map<String, FlinkMetric>>
filteredVertexMetricNames);
- public void cleanup(AbstractFlinkResource<?, ?> cr) {
- var resourceId = ResourceID.fromResource(cr);
- histories.remove(resourceId);
- availableVertexMetricNames.remove(resourceId);
+ public JobDetailsInfo getJobDetailsInfo(
+ JobAutoScalerContext<KEY> context, Duration clientTimeout) throws
Exception {
Review Comment:
Thanks @mateczagany for this comment.
IIUC, you mean `ScalingMetricCollector` is using the `RestClusterClient`,
and `RestApiMetricsCollector` is totally based on `RestClusterClient`, so these
2 classes can be merged into one classes, right?
If so, I try to explain the difference between : `RestApiMetricsCollector`
and `ScalingMetricCollector`.
- `RestApiMetricsCollector` calls `RestClusterClient`, and it's used to
fetch specific metrics.
- `ScalingMetricCollector` calls `RestClusterClient` and is not used to
fetch specific metrics.
- `RestClusterClient` is used in `ScalingMetricCollector` to get some job
metadata, such as: `getJobDetailsInfo` to generate the `JobTopology`,
`queryFilteredMetricNames`, `updateKafkaSourceMaxParallelisms`.
- The JobTopology is the metadata of Job, and it cannot be fetched from
metrics. That means the `RestClusterClient` is needed even if we query specific
metrics from other system.
Based on them, it may be better to keep `ScalingMetricCollector` as abstract
class and not remove `RestApiMetricsCollector`. It's easy to fetch specific
metrics from other system in the future.
Also, we can see the `ScalingMetricCollector` also used the
`RestClusterClient` on the current master branch.
WDYT? And please correct me if my understanding is wrong, thanks~
[1]
https://github.com/apache/flink-kubernetes-operator/blob/305498a9ab2e04ab71a4c2d87f2edb746373df1a/flink-kubernetes-operator-autoscaler/src/main/java/org/apache/flink/kubernetes/operator/autoscaler/ScalingMetricCollector.java#L368C45-L368C45
--
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]
