I think Dawid raised a very good point here. One of the outcomes should be that we are consistent in our recommendations and requests during PR reviews. Otherwise we'll just confuse contributors.
So I would be +1 for someone to use Optional in a private method if they believe it is helpful -1 to push contributors during reviews to do that On Tue, Aug 20, 2019 at 9:42 AM Dawid Wysakowicz <dwysakow...@apache.org> wrote: > Hi Andrey, > > Just wanted to quickly elaborate on my opinion. I wouldn't say I am -1, > just -0 for the Optionals in private methods. I am ok with not > forbidding them there. I just think in all cases there is a better > solution than passing the Optionals around, even in private methods. I > just hope the outcome of the discussion won't be that it is no longer > allowed to suggest those during review. > > Best, > > Dawid > > On 19/08/2019 17:53, Andrey Zagrebin 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 > >>>>>> > >>>>>> > >>>> > >> > >
