[GitHub] flink issue #3106: [FLINK-5118] Fix inconsistent numBytesIn/Out metrics

2017-01-12 Thread uce
Github user uce commented on the issue: https://github.com/apache/flink/pull/3106 Good catch. I was wondering whether it is better to count in a consistent way, too, e.g. count the size of each buffer or each record on both A and B (now we have per record on the output side and per bu

[GitHub] flink issue #3106: [FLINK-5118] Fix inconsistent numBytesIn/Out metrics

2017-01-12 Thread zentol
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3106 Instead of counting bytesOut in the serializer we could do it one level up in the RecordWriter. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub

[GitHub] flink issue #3106: [FLINK-5118] Fix inconsistent numBytesIn/Out metrics

2017-01-12 Thread zentol
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3106 Then we could count per buffer. I'd rather not count per record since the deserializer are kind of complicated :( --- If your project is set up for it, you can reply to this email and have your reply

[GitHub] flink issue #3106: [FLINK-5118] Fix inconsistent numBytesIn/Out metrics

2017-01-12 Thread uce
Github user uce commented on the issue: https://github.com/apache/flink/pull/3106 Yeah, I think counting the buffers is the better way to go. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have t

[GitHub] flink issue #3106: [FLINK-5118] Fix inconsistent numBytesIn/Out metrics

2017-01-13 Thread zentol
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3106 @uce Could you take another look? I moved the bytesOut counter into the RecordWriter. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well.

[GitHub] flink issue #3106: [FLINK-5118] Fix inconsistent numBytesIn/Out metrics

2017-01-15 Thread xhumanoid
Github user xhumanoid commented on the issue: https://github.com/apache/flink/pull/3106 @zentol what do you think about remove if (numBytesOut != null) { and replace numBytesOut = metrics.getNumBytesOutCounter(); with + if (metrics.getNumBytesOu

[GitHub] flink issue #3106: [FLINK-5118] Fix inconsistent numBytesIn/Out metrics

2017-01-15 Thread zentol
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3106 @xhumanoid The metrics returned by the TaskIOMetricGroup can't actually be null, so I wouldn't put too much thought into dealing with that case. --- If your project is set up for it, you can reply to

[GitHub] flink issue #3106: [FLINK-5118] Fix inconsistent numBytesIn/Out metrics

2017-01-15 Thread xhumanoid
Github user xhumanoid commented on the issue: https://github.com/apache/flink/pull/3106 @zentol I asked because you always check on null when you try writing to Counter or is it prevent uninitialized state? --- If your project is set up for it, you can reply to this email and ha

[GitHub] flink issue #3106: [FLINK-5118] Fix inconsistent numBytesIn/Out metrics

2017-01-16 Thread zentol
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3106 The code checks for null since there is no *technical* contract that the returned value is null. They aren't strictly necessary, and are only meant to guard against programming errors in the metrics s

[GitHub] flink issue #3106: [FLINK-5118] Fix inconsistent numBytesIn/Out metrics

2017-01-16 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3106 +1 to this approach. Some suggestions for small performance improvements, not specific to this case, but also applicable to other cases: - It may make sense to type the counters at

[GitHub] flink issue #3106: [FLINK-5118] Fix inconsistent numBytesIn/Out metrics

2017-01-17 Thread zentol
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3106 @StephanEwen I'll add the default initialization of ```counter``` while merging. Typing the counters to a ```SimpleCounter``` would affect a lot of other classes though (especially since we c