[GitHub] flink issue #4647: [FLINK-7575] [WEB-DASHBOARD] Display "Fetching..." instea...
Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/4647 The next time we touch `MutableIOMetrics`, we should also refactor `MutableIOMetrics#addIOMetrics` such that one passes an IOMetrics object into it instead of the `MetricFetcher`. ---
[GitHub] flink issue #4647: [FLINK-7575] [WEB-DASHBOARD] Display "Fetching..." instea...
Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/4647 Sorry for joining the party so late, but I think the solution is incomplete. Flink displays many metrics for the different components of the system. For example, the `TaskManagersHandler` displays the memory of the respective `TaskManagers`. If we say, we don't want to display metric information if it's not complete, then this should be fixed as well. ---
[GitHub] flink issue #4647: [FLINK-7575] [WEB-DASHBOARD] Display "Fetching..." instea...
Github user zentol commented on the issue: https://github.com/apache/flink/pull/4647 merging. ---
[GitHub] flink issue #4647: [FLINK-7575] [WEB-DASHBOARD] Display "Fetching..." instea...
Github user jameslafa commented on the issue: https://github.com/apache/flink/pull/4647 @zentol I agree, it make it a lot easier to read. I just pushed the update. Thanks for your feedback. ---
[GitHub] flink issue #4647: [FLINK-7575] [WEB-DASHBOARD] Display "Fetching..." instea...
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4647
Tried it out and it works as expected.
After looking at the aggregation code in detail I suggest to set an
isComplete boolean for each counter instead of setting the value to -1. This
makes things more explicit, and preserves the existing behavior of happily
aggregating the counters. It also simplifies the addition of
bytesInLocal/-Remote.
I.e., the aggregation part looks like this:
```
String numBytesInRemoteString =
metrics.getMetric(MetricNames.IO_NUM_BYTES_IN_REMOTE);
if (numBytesInRemoteString == null) {
this.numBytesInRemoteComplete = false;
} else {
this.numBytesInRemote += Long.valueOf(numBytesInRemoteString);
}
```
and the writing like this:
```
public void writeIOMetricsAsJson(JsonGenerator gen) throws IOException {
gen.writeObjectFieldStart("metrics");
long numBytesIn = this.numBytesInLocal + this.numBytesInRemote;
writeIOMetricWithCompleteness(gen, "read-bytes", numBytesIn,
this.numBytesInLocalComplete && this.numBytesInRemoteComplete);
writeIOMetricWithCompleteness(gen, "write-bytes", this.numBytesOut,
this.numBytesOutComplete);
writeIOMetricWithCompleteness(gen, "read-records", this.numRecordsIn,
this.numRecordsInComplete);
writeIOMetricWithCompleteness(gen, "write-records", this.numRecordsOut,
this.numRecordsOutComplete);
gen.writeEndObject();
}
private void writeIOMetricWithCompleteness(JsonGenerator gen, String
fieldName, long fieldValue, boolean isComplete) throws IOException{
gen.writeNumberField(fieldName, fieldValue);
gen.writeBooleanField(fieldName + "-complete", isComplete);
}
```
What do you think @jameslafa ?
---
[GitHub] flink issue #4647: [FLINK-7575] [WEB-DASHBOARD] Display "Fetching..." instea...
Github user jameslafa commented on the issue: https://github.com/apache/flink/pull/4647 @zentol I pushed the changes we talk about yesterday. Let me know if it's fine for you :) Thanks for your help. ---
[GitHub] flink issue #4647: [FLINK-7575] [WEB-DASHBOARD] Display "Fetching..." instea...
Github user zentol commented on the issue: https://github.com/apache/flink/pull/4647 @uce The checkstyle didn't catch it since the `runtime/executiongraph` package isn't covered by checkstyle. The flag will not fulfill the intended purpose after #4472 since the fetched metrics will be inserted one by one and not atomically in batches (neither as a whole nor by job/task/operator). The same issue applies to the current master since `MutableIOMetric` doesn't synchronize on the metric store. For the flag to properly work you will have to verify that every single metric that you access from the store is not null. If any metric is null the update isn't complete and you send back -1. ---
[GitHub] flink issue #4647: [FLINK-7575] [WEB-DASHBOARD] Display "Fetching..." instea...
Github user uce commented on the issue: https://github.com/apache/flink/pull/4647 Thanks @zentol for the review. @jameslafa - Regarding the first point with checkstyle: It's confusing that our checkstyle settings don't catch this, but Chesnay is right here. Seems nitpicky but we try to avoid unnecessary formatting changes. - Regarding the changes to the *.js files: Since you didn't change any of the coffee scripts, there should be no need to commit those files and I would also suggest to remove those changes. The changes are probably due to different versions on your machine and the previous contributor who changed the files. I think this only reinforces the argument we had about committing the *.lock file too. Could you create a new JIRA for this? @zentol - I didn't understand your follow up comments regarding the `metricsFetched` flag. Could you please elaborate on what you mean? Is the flag ok to keep after #4472 is merged? ---
