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