FYI the PR: https://github.com/apache/flink-web/pull/251 A review from the discussion participants would be appreciated.
On Wed, Aug 21, 2019 at 1:23 PM Timo Walther <twal...@apache.org> wrote: > Thanks for summarizing the discussion Andrey, +1 to this style. > > Regards, > Timo > > > Am 21.08.19 um 11:57 schrieb Andrey Zagrebin: > > Hi All, > > > > It looks like we have reached a consensus regarding the last left > question. > > > > I suggest the following final summary: > > > > - Use @Nullable annotation where you do not use Optional for the > > nullable values > > - If you can prove that Optional usage would lead to a performance > > degradation in critical code then fallback to @Nullable > > - Always use Optional to return nullable values in the API/public > > methods except the case of a proven performance concern > > - Do not use Optional as a function argument, instead either overload > > the method or use the Builder pattern for the set of function > arguments > > - Note: an Optional argument can be allowed in a *private* helper > > method if you believe that it simplifies the code, example is in > [1] > > - Do not use Optional for class fields > > > > If there are no more comments/concerns/objections I will open a PR to > > reflect this in the code style guide. > > > > Bets, > > Andrey > > > > [1] > > > https://github.com/apache/flink/blob/master/flink-formats/flink-avro/src/main/java/org/apache/flink/formats/avro/typeutils/AvroFactory.java#L95 > > > > On Tue, Aug 20, 2019 at 10:35 AM Yu Li <car...@gmail.com> wrote: > > > >> 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 > >>>>>>>>>> > >>>> > >
