@apilloud I assume you've looked at the updated PR. > LGTM, but there is an implied // this should never happen here.
I don't think that's an implication with `.hasNext()`. "`.hasNext()+.next()`" is the contract of the iterator interface and it's a good practice to guard `.next()` with `.hasNext()`. In this case it's similar to if you needed to check `if (counters.size() > 0)` before `counters.get(0)`, which you probably want to always check unless there's clear guarantee of non-emptiness. In this case there's no clear guarantee of the non-emptiness so I think that it's correct to handle the emptiness. I don't know whether lack of metrics always means that `count = 0`, but to me it seems reasonable to assume this. I agree that it's unclear from just reading this line of code what the behavior of metrics is expected to be for an empty pipeline, or if there are other cases when metrics can be empty. My opinion though is that ambiguity in this case is caused by the fact that iterator is picked as an interface to extract metrics and not by how we handle it. [ Full content available at: https://github.com/apache/beam/pull/6309 ] This message was relayed via gitbox.apache.org for [email protected]
