You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "mxm (via GitHub)" <gi...@apache.org> on 2023/03/02 12:59:41 UTC

[GitHub] [flink-kubernetes-operator] mxm opened a new pull request, #542: [FLINK-31299] PendingRecords metric might not be available

mxm opened a new pull request, #542:
URL: https://github.com/apache/flink-kubernetes-operator/pull/542

   The Kafka pendingRecords metric is only initialized on receiving the first record. For empty topics or checkpointed topics without any incoming data, the metric won't appear.
   
   We need to handle this case in the autoscaler and allow downscaling.


-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [flink-kubernetes-operator] gyfora commented on a diff in pull request #542: [FLINK-31299] PendingRecords metric might not be available

Posted by "gyfora (via GitHub)" <gi...@apache.org>.
gyfora commented on code in PR #542:
URL: https://github.com/apache/flink-kubernetes-operator/pull/542#discussion_r1123404812


##########
flink-kubernetes-operator-autoscaler/src/main/java/org/apache/flink/kubernetes/operator/autoscaler/metrics/ScalingMetrics.java:
##########
@@ -156,6 +156,8 @@ public static void computeLagMetrics(
         var pendingRecords = flinkMetrics.get(FlinkMetric.PENDING_RECORDS);
         if (pendingRecords != null) {
             scalingMetrics.put(ScalingMetric.LAG, pendingRecords.getSum());
+        } else {
+            scalingMetrics.put(ScalingMetric.LAG, 0.);

Review Comment:
   Ok, in that case can you please remove the parts that are not necessary anymore?



-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [flink-kubernetes-operator] gyfora commented on a diff in pull request #542: [FLINK-31299] PendingRecords metric might not be available

Posted by "gyfora (via GitHub)" <gi...@apache.org>.
gyfora commented on code in PR #542:
URL: https://github.com/apache/flink-kubernetes-operator/pull/542#discussion_r1123261513


##########
flink-kubernetes-operator-autoscaler/src/main/java/org/apache/flink/kubernetes/operator/autoscaler/metrics/ScalingMetrics.java:
##########
@@ -156,6 +156,8 @@ public static void computeLagMetrics(
         var pendingRecords = flinkMetrics.get(FlinkMetric.PENDING_RECORDS);
         if (pendingRecords != null) {
             scalingMetrics.put(ScalingMetric.LAG, pendingRecords.getSum());
+        } else {
+            scalingMetrics.put(ScalingMetric.LAG, 0.);

Review Comment:
   Wouldn't this logic affect sources that do not report the pending records metric at all ?
   Those currently would go through a different codepath in: https://github.com/apache/flink-kubernetes-operator/blob/main/flink-kubernetes-operator-autoscaler/src/main/java/org/apache/flink/kubernetes/operator/autoscaler/metrics/ScalingMetrics.java#L111
   



-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [flink-kubernetes-operator] mxm commented on a diff in pull request #542: [FLINK-31299] PendingRecords metric might not be available

Posted by "mxm (via GitHub)" <gi...@apache.org>.
mxm commented on code in PR #542:
URL: https://github.com/apache/flink-kubernetes-operator/pull/542#discussion_r1123402844


##########
flink-kubernetes-operator-autoscaler/src/main/java/org/apache/flink/kubernetes/operator/autoscaler/metrics/ScalingMetrics.java:
##########
@@ -156,6 +156,8 @@ public static void computeLagMetrics(
         var pendingRecords = flinkMetrics.get(FlinkMetric.PENDING_RECORDS);
         if (pendingRecords != null) {
             scalingMetrics.put(ScalingMetric.LAG, pendingRecords.getSum());
+        } else {
+            scalingMetrics.put(ScalingMetric.LAG, 0.);

Review Comment:
   I think both can share the same logic now. The linked code may be dead code now.



-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [flink-kubernetes-operator] mxm commented on a diff in pull request #542: [FLINK-31299] PendingRecords metric might not be available

Posted by "mxm (via GitHub)" <gi...@apache.org>.
mxm commented on code in PR #542:
URL: https://github.com/apache/flink-kubernetes-operator/pull/542#discussion_r1126666670


##########
flink-kubernetes-operator-autoscaler/src/main/java/org/apache/flink/kubernetes/operator/autoscaler/metrics/ScalingMetrics.java:
##########
@@ -156,6 +156,8 @@ public static void computeLagMetrics(
         var pendingRecords = flinkMetrics.get(FlinkMetric.PENDING_RECORDS);
         if (pendingRecords != null) {
             scalingMetrics.put(ScalingMetric.LAG, pendingRecords.getSum());
+        } else {
+            scalingMetrics.put(ScalingMetric.LAG, 0.);

Review Comment:
   I'll follow up with a cleanup PR.



-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [flink-kubernetes-operator] mxm merged pull request #542: [FLINK-31299] PendingRecords metric might not be available

Posted by "mxm (via GitHub)" <gi...@apache.org>.
mxm merged PR #542:
URL: https://github.com/apache/flink-kubernetes-operator/pull/542


-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org