[DISCUSS][CODE STYLE] Usage of Java Optional

[Andrey Zagrebin-3]

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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]
> >
> > 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 <[hidden email]>
> > > 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 <[hidden email]>
> 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 <
> > [hidden email]>
> > > >>> 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 <[hidden email]> 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 <
> [hidden email]
> > > >>>> .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 <[hidden email]>
> > > >>>>>> Send Time:2019年8月2日(星期五) 14:12
> > > >>>>>> To:dev <[hidden email]>
> > > >>>>>> 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 <[hidden email]
> >
> > > >> 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 <[hidden email]> 于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 <[hidden email]>
> > > 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 <[hidden email]>
> > > >> 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 <
> > > >>>> [hidden email]
> > > >>>>>>>>>>> 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-85BC710FD157@...%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
> > > >>>>>>
> > > >>>>>>
> > > >>>>
> > > >>
> > >
> > >
> >
>

[Timo Walther-2]

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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]
>>>
>>> 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 <[hidden email]>
>>>> 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 <[hidden email]>
>> 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 <
>>> [hidden email]>
>>>>>>> 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 <[hidden email]> 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 <
>> [hidden email]
>>>>>>>> .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 <[hidden email]>
>>>>>>>>>> Send Time:2019年8月2日(星期五) 14:12
>>>>>>>>>> To:dev <[hidden email]>
>>>>>>>>>> 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 <[hidden email]
>>>>>> 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 <[hidden email]> 于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 <[hidden email]>
>>>> 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 <[hidden email]>
>>>>>> 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 <
>>>>>>>> [hidden email]
>>>>>>>>>>>>>>> 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-85BC710FD157@...%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
>>>>>>>>>>
>>>>

[Andrey Zagrebin-3]

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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <
> [hidden email]
> >>>
> >>> 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 <[hidden email]>
> >>>> 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 <[hidden email]>
> >> 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 <
> >>> [hidden email]>
> >>>>>>> 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 <[hidden email]> 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 <
> >> [hidden email]
> >>>>>>>> .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 <[hidden email]>
> >>>>>>>>>> Send Time:2019年8月2日(星期五) 14:12
> >>>>>>>>>> To:dev <[hidden email]>
> >>>>>>>>>> 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 <[hidden email]
> >>>>>> 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 <[hidden email]> 于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 <[hidden email]>
> >>>> 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 <[hidden email]>
> >>>>>> 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 <
> >>>>>>>> [hidden email]
> >>>>>>>>>>>>>>> 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-85BC710FD157@...%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
> >>>>>>>>>>
> >>>>
>
>

[tison]

Thanks for driving this thread Andrey. Conclusion looks good to me.

Best,
tison.


Andrey Zagrebin <[hidden email]> 于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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <
> > [hidden email]
> > >>>
> > >>> 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 <[hidden email]
> >
> > >>>> 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 <[hidden email]>
> > >> 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 <
> > >>> [hidden email]>
> > >>>>>>> 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 <[hidden email]> 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 <
> > >> [hidden email]
> > >>>>>>>> .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 <[hidden email]>
> > >>>>>>>>>> Send Time:2019年8月2日(星期五) 14:12
> > >>>>>>>>>> To:dev <[hidden email]>
> > >>>>>>>>>> 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 <
> [hidden email]
> > >>>>>> 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 <[hidden email]> 于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 <[hidden email]>
> > >>>> 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 <
> [hidden email]>
> > >>>>>> 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 <
> > >>>>>>>> [hidden email]
> > >>>>>>>>>>>>>>> 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-85BC710FD157@...%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
> > >>>>>>>>>>
> > >>>>
> >
> >
>