[BEAM-3081] NonNull by default in the rest of the core SDK and shared runner code #4058
Conversation
kennknowles
force-pushed
the
NonNull-by-default
branch
6 times, most recently
from
3eef21a
to
1a26480
Compare
|
run java precommit |
kennknowles
force-pushed
the
NonNull-by-default
branch
2 times, most recently
from
6b4518f
to
e221627
Compare
|
R: @jkff another :-) |
| @@ -27,11 +28,13 @@ | |||
| public class FlinkNoOpStepContext implements StepContext { | |||
|
|
|||
| @Override | |||
| @Nullable | |||
There was a problem hiding this comment.
Should these also throw an exception?
| @@ -229,7 +230,9 @@ public void populateDisplayData(DisplayData.Builder builder) { | |||
|
|
|||
| private class Reader extends BoundedReader<ValueWithRecordId<T>> { | |||
| private long recordsRead = 0L; | |||
| private Instant endTime = Instant.now().plus(getMaxReadTime()); | |||
|
|
|||
| @Nullable private Instant endTime = Instant.now().plus(getMaxReadTime()); | |||
There was a problem hiding this comment.
It turns out that it does "work": "If the amount is zero or null, then this is returned.". Certainly not the spec I would have chosen. Of course, if it didn't work we would have crashes already.
On the other hand, this initial value is always overwritten in the constructor in a way that contradicts the logic here. So I've just removed this.
| @@ -259,6 +259,10 @@ public static ResourceId convertToFileResourceIfPossible(String outputPrefix) { | |||
| * #getSideInputs()}. | |||
| */ | |||
| protected final <SideInputT> SideInputT sideInput(PCollectionView<SideInputT> view) { | |||
| checkState( | |||
| sideInputAccessor != null, | |||
| "sideInput called on %s but side inputs" + " have not been initialized", | |||
| private byte[] currentValue; | ||
| private ReadableByteChannel inChannel; | ||
| private TFRecordCodec codec; | ||
| private @Nullable byte[] currentValue; |
There was a problem hiding this comment.
We have inconsistent placement of @Nullable - sometimes before the access modifier, sometimes after. Is it an issue?
There was a problem hiding this comment.
Google style, which we tend to follow, indicates
Annotations applying to a class, method or constructor[, or field] appear immediately after the documentation block
...
There are no specific rules for formatting annotations on parameters, local variables, or types.
I view @Nullable as always applying to a type, consistent with Java 8+ pluggable type systems, and more future-proof because it actually makes sense. In Java 7, applying @Nullable to a method is a necessary hack implying that the return type was @Nullable. You could debate whether it applies to the field or the type of the field. In all these cases, the actual lexemes can be ordered in any way with the same effect. I regret the many times I have written these in the wrong order. Perhaps I will do a global search and replace some day.
kennknowles
force-pushed
the
NonNull-by-default
branch
from
ed305a7
to
87b07d8
Compare
…core SDK and shared runner code NonNull by default in runners/core NonNull by default in runners/core/metrics NonNull by default in runners/core/fn NonNull by default in runners/core/triggers NonNull by default in runners/core/construction/metrics NonNull by default in runners/core/construction NonNull by default in sdk/io/range NonNull by default in sdk/io/fs NonNull by default in sdk/values NonNull by default in sdk/io
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.