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