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 > > > > > > > > >
