Thanks for the summarize Andrey! I'd also like to adjust my -1 to +0 on using Optional as parameter for private methods due to the existence of the very first rule - "Avoid using Optional in any performance critical code". I'd regard the "possible GC burden while using Optional as parameter" also one performance related factor.
And besides the code convention itself, I believe it's even more important to make us contributors know the reason behind. Thanks. Best Regards, Yu On Tue, 20 Aug 2019 at 10:14, Stephan Ewen <se...@apache.org> wrote: > 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 > > >>>>>> > > >>>>>> > > >>>> > > >> > > > > >
