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 >>>>>> >>>>>> >>>> >>
signature.asc
Description: OpenPGP digital signature
