You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2021/01/04 15:01:31 UTC
[GitHub] [flink] pnowojski commented on a change in pull request #14526: [FLINK-14814][WebUI] Highlight back pressured and busy nodes in the WebUI
pnowojski commented on a change in pull request #14526:
URL: https://github.com/apache/flink/pull/14526#discussion_r551350494
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/TimeGauge.java
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+// ----------------------------------------------------------------------------
+// This class is largely adapted from "com.google.common.base.Preconditions",
Review comment:
ops, I copied pasted `MeterView` and missed this comment. Removed.
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/TimeGauge.java
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+// ----------------------------------------------------------------------------
+// This class is largely adapted from "com.google.common.base.Preconditions",
+// which is part of the "Guava" library.
+//
+// Because of frequent issues with dependency conflicts, this class was
+// added to the Flink code base to reduce dependency on Guava.
+// ----------------------------------------------------------------------------
+
+package org.apache.flink.runtime.metrics.groups;
+
+import org.apache.flink.annotation.VisibleForTesting;
+import org.apache.flink.metrics.Gauge;
+import org.apache.flink.metrics.View;
+
+/**
+ * {@link TimeGauge} encapsulate logic of measuring time rates (like idleTime ms/s). In particularly
Review comment:
I think this was unconscious decision. It was kind of natural from the perspective that we are measuring `idleTime` but the `MeterView` is exposing it a `ms/s` rate.
But yes, maybe percentage would have been better? Do you think it's worth the hassle of changing the API at this point of time?
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/TimeGauge.java
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+// ----------------------------------------------------------------------------
+// This class is largely adapted from "com.google.common.base.Preconditions",
+// which is part of the "Guava" library.
+//
+// Because of frequent issues with dependency conflicts, this class was
+// added to the Flink code base to reduce dependency on Guava.
+// ----------------------------------------------------------------------------
+
+package org.apache.flink.runtime.metrics.groups;
+
+import org.apache.flink.annotation.VisibleForTesting;
+import org.apache.flink.metrics.Gauge;
+import org.apache.flink.metrics.View;
+
+/**
+ * {@link TimeGauge} encapsulate logic of measuring time rates (like idleTime ms/s). In particularly
+ * it takes care of the case, when single measurement (as defined by {@link #markStart()} and {@link
+ * #markEnd()}), crosses the boundary of single {@link #update()} interval. Thanks to the {@link
+ * #markStart()} {@link TimeGauge} can correctly calculate next value in {@link #update()}, even if
+ * this measurement hasn't not ended ({@link #markEnd()}).
+ */
+public class TimeGauge implements Gauge<Long>, View {
+ private long previousCount;
+ private long currentCount;
+ private long lastDifference;
+ private long currentMeasurementStart;
+
+ public synchronized void markStart() {
+ if (currentMeasurementStart == 0) {
+ currentMeasurementStart = System.currentTimeMillis();
+ }
+ }
+
+ public synchronized void markEnd() {
+ if (currentMeasurementStart != 0) {
+ currentCount += System.currentTimeMillis() - currentMeasurementStart;
+ currentMeasurementStart = 0;
+ }
+ }
+
+ @Override
+ public synchronized void update() {
+ if (currentMeasurementStart != 0) {
+ long now = System.currentTimeMillis();
+ currentCount += now - currentMeasurementStart;
+ currentMeasurementStart = now;
+ }
+ lastDifference = (currentCount - previousCount) / UPDATE_INTERVAL_SECONDS;
+ lastDifference = Math.max(Math.min(lastDifference, 1000), 0);
+ previousCount = currentCount;
Review comment:
Yes we could :) Thanks for the suggestion.
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/TimeGauge.java
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+// ----------------------------------------------------------------------------
+// This class is largely adapted from "com.google.common.base.Preconditions",
+// which is part of the "Guava" library.
+//
+// Because of frequent issues with dependency conflicts, this class was
+// added to the Flink code base to reduce dependency on Guava.
+// ----------------------------------------------------------------------------
+
+package org.apache.flink.runtime.metrics.groups;
+
+import org.apache.flink.annotation.VisibleForTesting;
+import org.apache.flink.metrics.Gauge;
+import org.apache.flink.metrics.View;
+
+/**
+ * {@link TimeGauge} encapsulate logic of measuring time rates (like idleTime ms/s). In particularly
+ * it takes care of the case, when single measurement (as defined by {@link #markStart()} and {@link
+ * #markEnd()}), crosses the boundary of single {@link #update()} interval. Thanks to the {@link
+ * #markStart()} {@link TimeGauge} can correctly calculate next value in {@link #update()}, even if
+ * this measurement hasn't not ended ({@link #markEnd()}).
Review comment:
I've rephrased the comment. Could you take a look? Also I have renamed `TimeGauge` to `TimerGauge`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org