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
> > > >>>>>>
> > > >>>>>>
> > > >>>>
> > > >>
> > >
> > >
> >
>
