BEAM-3803: Dataflow runner implements metrics contract#4841
BEAM-3803: Dataflow runner implements metrics contract#4841kennknowles merged 2 commits intoapache:masterfrom
Conversation
apilloud
changed the title
|
Strong agreement. If you have committed metrics, that number is clearly within spec for attempted metrics (which can be pretty much any value at all, when you get down to it). |
| public T committed() { | ||
| T committed = committedInternal(); | ||
| if (committed == null) { | ||
| throw new UnsupportedOperationException("This runner does not currently support committed" |
There was a problem hiding this comment.
I'm not so keen on the use of exception instead of null. If something is explicitly optional @Nullable seems reasonable (insert standard rant about how Optional is a better choice but Java screwed it up and Guava is shaded).
I would imagine that changes to this class would just remove @Nullable from attempted, then an additional static factory method like createCommitted(<just committed value>) that populates both fields with that value, createAttempted(<just attempted value>) that leaves committed null and create(<both not nullable>). Incidentally the existing create method seems broken as it fails to mark them as @Nullable. When we plug in the checker framework that should be caught.
There was a problem hiding this comment.
I see - the javadoc tells you to do this. That sucks. Want to fix the javadoc to have this better spec?
There was a problem hiding this comment.
OK let's just make Dataflow match the spec and later change the spec to be good.
|
run java precommit |
1 similar comment
|
run java precommit |
|
run java gradle precommit |
|
run java precommit |
2 participants
Attempted metrics are not optional, they should always be returned. Committed metrics are a close enough approximation for attempted. When attempted metrics are not supported, a
UnsupportedOperationExceptionshould be thrown rather then returning null. See:beam/sdks/java/core/src/main/java/org/apache/beam/sdk/metrics/MetricResult.java
Line 42 in 3c73170
Follow this checklist to help us incorporate your contribution quickly and easily:
[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue.mvn clean verifyto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.