Hi all, Sorry for not getting back to this discussion for some time. It looks like in general we agree on the initially suggested points:
- Always use Optional only to return nullable values in the API/public methods - Only if you can prove that Optional usage would lead to a performance degradation in critical code then return nullable value directly and annotate it with @Nullable - Passing an Optional argument to a method can be allowed if it is within a private helper method and simplifies the code - Optional should not be used for class fields The first point can be also elaborated by explicitly forbiding Optional/Nullable parameters in public methods. In general we can always avoid Optional parameters by either overloading the method or using a pojo with a builder to pass a set of parameters. The third point does not prevent from using @Nullable/@Nonnull for class fields. If we agree to not use Optional for fields then not sure I see any use case for SerializableOptional (please comment on that if you have more details). @Jingsong Lee Using Optional in Maps. I can see this as a possible use case. I would leave this decision to the developer/reviewer to reason about it and keep the scope of this discussion to the variables/parameters as it seems to be the biggest point of friction atm. Finally, I see a split regarding the second point: <Passing an Optional argument to a method can be allowed if it is within a private helper method and simplifies the code>. There are people who have a strong opinion against allowing it. Let's vote then for whether to allow it or not. So far as I see we have the following votes (correct me if wrong and add more if you want): Piotr +1 Biao +1 Timo -1 Yu -1 Aljoscha -1 Till +1 Igal +1 Dawid -1 Me +1 So far: +5 / -4 (Optional arguments in private methods) Best, Andrey On Tue, Aug 6, 2019 at 8:53 AM Piotr Nowojski <pi...@ververica.com> wrote: > Hi Qi, > > > 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? > > When I was introducing it there, I have micro-benchmarked this change, and > there was no visible throughput change in a pure network only micro > benchmark (with whole Flink running around it any changes would be even > less visible). > > Piotrek > > > On 5 Aug 2019, at 15:20, Till Rohrmann <trohrm...@apache.org> wrote: > > > > I'd be in favour of > > > > - Optional for method return values if not performance critical > > - Optional can be used for internal methods if it makes sense > > - No optional fields > > > > Cheers, > > Till > > > > On Mon, Aug 5, 2019 at 12:07 PM Aljoscha Krettek <aljos...@apache.org> > > wrote: > > > >> Hi, > >> > >> I’m also in favour of using Optional only for method return values. I > >> could be persuaded to allow them for parameters of internal methods but > I’m > >> sceptical about that. > >> > >> Aljoscha > >> > >>> On 2. Aug 2019, at 15:35, Yu Li <car...@gmail.com> wrote: > >>> > >>> 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 > >>>>>>>>> > >>>> > >>>> > >>>> > >> > >> > >