Thanks for driving this thread Andrey. Conclusion looks good to me. Best, tison.
Andrey Zagrebin <and...@ververica.com> 于2019年8月21日周三 下午7:25写道： > 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 > > >>>>>>>>>> > > >>>> > > > > >
