TL; DR: I second Timo that we should use Optional only as method return type for non-performance critical code.
>From the example given on our AvroFactory [1] I also noticed that Jetbrains marks the OptionalUsedAsFieldOrParameterType inspection as a warning. It's relatively easy to understand why it's not suggested to use (java.util) Optional as a field since it's not serializable. What made me feel curious is that why we shouldn't use it as a parameter type, so I did some investigation and here is what I found: There's a JB blog talking about java8 top tips [2] where we could find the advice around Optional, there I found another blog telling about the pragmatic approach of using Optional [3]. Reading further we could see the reason why we shouldn't use Optional as parameter type, please allow me to quote here: It is often the case that domain objects hang about in memory for a fair while, as processing in the application occurs, making each optional instance rather long-lived (tied to the lifetime of the domain object). By contrast, the Optionalinstance returned from the getter is likely to be very short-lived. The caller will call the getter, interpret the result, and then move on. If you know anything about garbage collection you'll know that the JVM handles these short-lived objects well. In addition, there is more potential for hotspot to remove the costs of the Optional instance when it is short lived. While it is easy to claim this is "premature optimization", as engineers it is our responsibility to know the limits and capabilities of the system we work with and to choose carefully the point where it should be stressed. And there's another JB blog about code smell on Null [4], which I'd also suggest to read(smile). [1] https://github.com/apache/flink/blob/master/flink-formats/flink-avro/src/main/java/org/apache/flink/formats/avro/typeutils/AvroFactory.java#L95 [2] https://blog.jetbrains.com/idea/2016/07/java-8-top-tips/ [3] https://blog.joda.org/2015/08/java-se-8-optional-pragmatic-approach.html [4] https://blog.jetbrains.com/idea/2017/08/code-smells-null/ Best Regards, Yu On Fri, 2 Aug 2019 at 14:54, JingsongLee <lzljs3620...@aliyun.com.invalid> wrote: > Hi, > First, Optional is just a wrapper, just like boxed value. So as long as > it's > not a field level operation, I think it is OK to performance. > I think guava optional has a good summary to the uses. [1] > > As a method return type, as an alternative to returning null to indicate > that no value was available > > To distinguish between "unknown" (for example, not present in a map) > and "known to have no value" > > To wrap nullable references for storage in a collection that does not > support > The latter two points seem reasonable, but they have few scenes. > > [1] > https://github.com/google/guava/blob/master/guava/src/com/google/common/base/Optional.java > > Best, > Jingsong Lee > > > ------------------------------------------------------------------ > From:Timo Walther <twal...@apache.org> > Send Time:2019年8月2日(星期五) 14:12 > To:dev <dev@flink.apache.org> > Subject:Re: [DISCUSS][CODE STYLE] Usage of Java Optional > > Hi everyone, > > I would vote for using Optional only as method return type for > non-performance critical code. Nothing more. No fields, no method > parameters. Method parameters can be overloaded and internally a class > can work with nulls and @Nullable. Optional is meant for API method > return types and I think we should not abuse it and spam the code with > `@SuppressWarnings("OptionalUsedAsFieldOrParameterType")`. > > Regards, > > Timo > > > > Am 02.08.19 um 11:08 schrieb Biao Liu: > > Hi Jark & Zili, > > > > I thought it means "Optional should not be used for class fields". > However > > now I get a bit confused about the edited version. > > > > Anyway +1 to "Optional should not be used for class fields" > > > > Thanks, > > Biao /'bɪ.aʊ/ > > > > > > > > On Fri, Aug 2, 2019 at 5:00 PM 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 > >>>>> > > >
