For the use of optional in private methods: It sounds fine to me, because it means it is strictly class-internal (between methods and helper methods) and does not leak beyond that.
On Mon, Aug 19, 2019 at 5:53 PM Andrey Zagrebin <and...@ververica.com> wrote: > 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 > > >>>>>>>>> > > >>>> > > >>>> > > >>>> > > >> > > >> > > > > >
