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