Hi Zili, Yes. I agree to use @Nullable/@Nonnull/SerializableOptional as the class field instead of Optional.
On Fri, 2 Aug 2019 at 17:00, Zili Chen <wander4...@gmail.com> wrote: > Hi Jark, > > Follow your opinion, for class field, we can make > use of @Nullable/@Nonnull annotation or Flink's > SerializableOptional. It would be sufficient. > > Best, > tison. > > > Jark Wu <imj...@gmail.com> 于2019年8月2日周五 下午4:57写道： > > > Hi Andrey, > > > > I have some concern on point (3) "even class fields as e.g. optional > config > > options with implicit default values". > > > > Regarding to the Oracle's guide (4) "Optional should not be used for > class > > fields". > > And IntelliJ IDEA also report warnings if a class field is Optional, > > because Optional is not serializable. > > > > > > Do we allow Optional as class field only if the class is not serializable > > or forbid this totally? > > > > Thanks, > > Jark > > > > On Fri, 2 Aug 2019 at 16:30, Biao Liu <mmyy1...@gmail.com> wrote: > > > > > Hi Andrey, > > > > > > Thanks for working on this. > > > > > > +1 it's clear and acceptable for me. > > > > > > To Qi, > > > > > > IMO the most performance critical codes are "per record" code path. We > > > should definitely avoid Optional there. For your concern, it's "per > > buffer" > > > code path which seems to be acceptable with Optional. > > > > > > Just one more question, is there any other code paths which are also > > > critical? I think we'd better note that clearly. > > > > > > Thanks, > > > Biao /'bɪ.aʊ/ > > > > > > > > > > > > On Fri, Aug 2, 2019 at 11:08 AM qi luo <luoqi...@gmail.com> wrote: > > > > > > > Agree that using Optional will improve code robustness. However we’re > > > > hesitating to use Optional in data intensive operations. > > > > > > > > For example, SingleInputGate is already creating Optional for every > > > > BufferOrEvent in getNextBufferOrEvent(). How much performance gain > > would > > > we > > > > get if it’s replaced by null check? > > > > > > > > Regards, > > > > Qi > > > > > > > > > On Aug 1, 2019, at 11:00 PM, Andrey Zagrebin <and...@ververica.com > > > > > > wrote: > > > > > > > > > > Hi all, > > > > > > > > > > This is the next follow up discussion about suggestions for the > > recent > > > > > thread about code style guide in Flink [1]. > > > > > > > > > > In general, one could argue that any variable, which is nullable, > can > > > be > > > > > replaced by wrapping it with Optional to explicitly show that it > can > > be > > > > > null. Examples are: > > > > > > > > > > - returned values to force user to check not null > > > > > - optional function arguments, e.g. with implicit default values > > > > > - even class fields as e.g. optional config options with implicit > > > > > default values > > > > > > > > > > > > > > > At the same time, we also have @Nullable annotation to express this > > > > > intention. > > > > > > > > > > Also, when the class Optional was introduced, Oracle posted a > > guideline > > > > > about its usage [2]. Basically, it suggests to use it mostly in > APIs > > > for > > > > > returned values to inform and force users to check the returned > value > > > > > instead of returning null and avoid NullPointerException. > > > > > > > > > > Wrapping with Optional also comes with the performance overhead. > > > > > > > > > > Following the Oracle's guide in general, the suggestion is: > > > > > > > > > > - Avoid using Optional in any performance critical code > > > > > - Use Optional only to return nullable values in the API/public > > > methods > > > > > unless it is performance critical then rather use @Nullable > > > > > - Passing an Optional argument to a method can be allowed if it > is > > > > > within a private helper method and simplifies the code, example > is > > in > > > > [3] > > > > > - Optional should not be used for class fields > > > > > > > > > > > > > > > Please, feel free to share you thoughts. > > > > > > > > > > Best, > > > > > Andrey > > > > > > > > > > [1] > > > > > > > > > > > > > > > http://mail-archives.apache.org/mod_mbox/flink-dev/201906.mbox/%3ced91df4b-7cab-4547-a430-85bc710fd...@apache.org%3E > > > > > [2] > > > > > > > > > > > > > > > https://www.oracle.com/technetwork/articles/java/java8-optional-2175753.html > > > > > [3] > > > > > > > > > > > > > > > https://github.com/apache/flink/blob/master/flink-formats/flink-avro/src/main/java/org/apache/flink/formats/avro/typeutils/AvroFactory.java#L95 > > > > > > > > > > > > > >
