Thanks for summarizing the discussion Andrey, +1 to this style.
Regards,
Timo
Am 21.08.19 um 11:57 schrieb Andrey Zagrebin:
Hi All,
It looks like we have reached a consensus regarding the last left question.
I suggest the following final summary:
- Use @Nullable annotation where you do not use Optional for the
nullable values
- If you can prove that Optional usage would lead to a performance
degradation in critical code then fallback to @Nullable
- Always use Optional to return nullable values in the API/public
methods except the case of a proven performance concern
- Do not use Optional as a function argument, instead either overload
the method or use the Builder pattern for the set of function arguments
- Note: an Optional argument can be allowed in a *private* helper
method if you believe that it simplifies the code, example is in [1]
- Do not use Optional for class fields
If there are no more comments/concerns/objections I will open a PR to
reflect this in the code style guide.
Bets,
Andrey
[1]
https://github.com/apache/flink/blob/master/flink-formats/flink-avro/src/main/java/org/apache/flink/formats/avro/typeutils/AvroFactory.java#L95
On Tue, Aug 20, 2019 at 10:35 AM Yu Li <car...@gmail.com> wrote:
Thanks for the summarize Andrey!
I'd also like to adjust my -1 to +0 on using Optional as parameter for
private methods due to the existence of the very first rule - "Avoid using
Optional in any performance critical code". I'd regard the "possible GC
burden while using Optional as parameter" also one performance related
factor.
And besides the code convention itself, I believe it's even more important
to make us contributors know the reason behind.
Thanks.
Best Regards,
Yu
On Tue, 20 Aug 2019 at 10:14, Stephan Ewen <se...@apache.org> wrote:
I think Dawid raised a very good point here.
One of the outcomes should be that we are consistent in our
recommendations
and requests during PR reviews. Otherwise we'll just confuse
contributors.
So I would be
+1 for someone to use Optional in a private method if they believe it
is
helpful
-1 to push contributors during reviews to do that
On Tue, Aug 20, 2019 at 9:42 AM Dawid Wysakowicz <dwysakow...@apache.org
wrote:
Hi Andrey,
Just wanted to quickly elaborate on my opinion. I wouldn't say I am -1,
just -0 for the Optionals in private methods. I am ok with not
forbidding them there. I just think in all cases there is a better
solution than passing the Optionals around, even in private methods. I
just hope the outcome of the discussion won't be that it is no longer
allowed to suggest those during review.
Best,
Dawid
On 19/08/2019 17:53, Andrey Zagrebin wrote:
Hi all,
Sorry for not getting back to this discussion for some time.
It looks like in general we agree on the initially suggested points:
- Always use Optional only to return nullable values in the
API/public
methods
- Only if you can prove that Optional usage would lead to a
performance degradation in critical code then return nullable
value
directly and annotate it with @Nullable
- Passing an Optional argument to a method can be allowed if it is
within a private helper method and simplifies the code
- Optional should not be used for class fields
The first point can be also elaborated by explicitly forbiding
Optional/Nullable parameters in public methods.
In general we can always avoid Optional parameters by either
overloading
the method or using a pojo with a builder to pass a set of
parameters.
The third point does not prevent from using @Nullable/@Nonnull for
class
fields.
If we agree to not use Optional for fields then not sure I see any
use
case
for SerializableOptional (please comment on that if you have more
details).
@Jingsong Lee
Using Optional in Maps.
I can see this as a possible use case.
I would leave this decision to the developer/reviewer to reason about
it
and keep the scope of this discussion to the variables/parameters as
it
seems to be the biggest point of friction atm.
Finally, I see a split regarding the second point: <Passing an
Optional
argument to a method can be allowed if it is within a private helper
method
and simplifies the code>.
There are people who have a strong opinion against allowing it.
Let's vote then for whether to allow it or not.
So far as I see we have the following votes (correct me if wrong and
add
more if you want):
Piotr +1
Biao +1
Timo -1
Yu -1
Aljoscha -1
Till +1
Igal +1
Dawid -1
Me +1
So far: +5 / -4 (Optional arguments in private methods)
Best,
Andrey
On Tue, Aug 6, 2019 at 8:53 AM Piotr Nowojski <pi...@ververica.com>
wrote:
Hi Qi,
For example, SingleInputGate is already creating Optional for every
BufferOrEvent in getNextBufferOrEvent(). How much performance gain
would we
get if it’s replaced by null check?
When I was introducing it there, I have micro-benchmarked this
change,
and
there was no visible throughput change in a pure network only micro
benchmark (with whole Flink running around it any changes would be
even
less visible).
Piotrek
On 5 Aug 2019, at 15:20, Till Rohrmann <trohrm...@apache.org>
wrote:
I'd be in favour of
- Optional for method return values if not performance critical
- Optional can be used for internal methods if it makes sense
- No optional fields
Cheers,
Till
On Mon, Aug 5, 2019 at 12:07 PM Aljoscha Krettek <
aljos...@apache.org>
wrote:
Hi,
I’m also in favour of using Optional only for method return
values.
I
could be persuaded to allow them for parameters of internal
methods
but
I’m
sceptical about that.
Aljoscha
On 2. Aug 2019, at 15:35, Yu Li <car...@gmail.com> wrote:
TL; DR: I second Timo that we should use Optional only as method
return
type for non-performance critical code.
From the example given on our AvroFactory [1] I also noticed that
Jetbrains
marks the OptionalUsedAsFieldOrParameterType inspection as a
warning.
It's
relatively easy to understand why it's not suggested to use
(java.util)
Optional as a field since it's not serializable. What made me
feel
curious
is that why we shouldn't use it as a parameter type, so I did
some
investigation and here is what I found:
There's a JB blog talking about java8 top tips [2] where we could
find
the
advice around Optional, there I found another blog telling about
the
pragmatic approach of using Optional [3]. Reading further we
could
see
the
reason why we shouldn't use Optional as parameter type, please
allow
me
to
quote here:
It is often the case that domain objects hang about in memory
for a
fair
while, as processing in the application occurs, making each
optional
instance rather long-lived (tied to the lifetime of the domain
object).
By
contrast, the Optionalinstance returned from the getter is likely
to
be
very short-lived. The caller will call the getter, interpret the
result,
and then move on. If you know anything about garbage collection
you'll
know
that the JVM handles these short-lived objects well. In addition,
there
is
more potential for hotspot to remove the costs of the Optional
instance
when it is short lived. While it is easy to claim this is
"premature
optimization", as engineers it is our responsibility to know the
limits
and
capabilities of the system we work with and to choose carefully
the
point
where it should be stressed.
And there's another JB blog about code smell on Null [4], which
I'd
also
suggest to read(smile).
[1]
https://github.com/apache/flink/blob/master/flink-formats/flink-avro/src/main/java/org/apache/flink/formats/avro/typeutils/AvroFactory.java#L95
[2] https://blog.jetbrains.com/idea/2016/07/java-8-top-tips/
[3]
https://blog.joda.org/2015/08/java-se-8-optional-pragmatic-approach.html
[4] https://blog.jetbrains.com/idea/2017/08/code-smells-null/
Best Regards,
Yu
On Fri, 2 Aug 2019 at 14:54, JingsongLee <
lzljs3620...@aliyun.com
.invalid>
wrote:
Hi,
First, Optional is just a wrapper, just like boxed value. So as
long
as
it's
not a field level operation, I think it is OK to performance.
I think guava optional has a good summary to the uses. [1]
As a method return type, as an alternative to returning null to
indicate
that no value was available
To distinguish between "unknown" (for example, not present in a
map)
and "known to have no value"
To wrap nullable references for storage in a collection that
does
not
support
The latter two points seem reasonable, but they have few scenes.
[1]
https://github.com/google/guava/blob/master/guava/src/com/google/common/base/Optional.java
Best,
Jingsong Lee
------------------------------------------------------------------
From:Timo Walther <twal...@apache.org>
Send Time:2019年8月2日(星期五) 14:12
To:dev <dev@flink.apache.org>
Subject:Re: [DISCUSS][CODE STYLE] Usage of Java Optional
Hi everyone,
I would vote for using Optional only as method return type for
non-performance critical code. Nothing more. No fields, no
method
parameters. Method parameters can be overloaded and internally a
class
can work with nulls and @Nullable. Optional is meant for API
method
return types and I think we should not abuse it and spam the
code
with
`@SuppressWarnings("OptionalUsedAsFieldOrParameterType")`.
Regards,
Timo
Am 02.08.19 um 11:08 schrieb Biao Liu:
Hi Jark & Zili,
I thought it means "Optional should not be used for class
fields".
However
now I get a bit confused about the edited version.
Anyway +1 to "Optional should not be used for class fields"
Thanks,
Biao /'bɪ.aʊ/
On Fri, Aug 2, 2019 at 5:00 PM Zili Chen <wander4...@gmail.com
wrote:
Hi Jark,
Follow your opinion, for class field, we can make
use of @Nullable/@Nonnull annotation or Flink's
SerializableOptional. It would be sufficient.
Best,
tison.
Jark Wu <imj...@gmail.com> 于2019年8月2日周五 下午4:57写道：
Hi Andrey,
I have some concern on point (3) "even class fields as e.g.
optional
config
options with implicit default values".
Regarding to the Oracle's guide (4) "Optional should not be
used
for
class
fields".
And IntelliJ IDEA also report warnings if a class field is
Optional,
because Optional is not serializable.
Do we allow Optional as class field only if the class is not
serializable
or forbid this totally?
Thanks,
Jark
On Fri, 2 Aug 2019 at 16:30, Biao Liu <mmyy1...@gmail.com>
wrote:
Hi Andrey,
Thanks for working on this.
+1 it's clear and acceptable for me.
To Qi,
IMO the most performance critical codes are "per record"
code
path.
We
should definitely avoid Optional there. For your concern,
it's
"per
buffer"
code path which seems to be acceptable with Optional.
Just one more question, is there any other code paths which
are
also
critical? I think we'd better note that clearly.
Thanks,
Biao /'bɪ.aʊ/
On Fri, Aug 2, 2019 at 11:08 AM qi luo <luoqi...@gmail.com>
wrote:
Agree that using Optional will improve code robustness.
However
we’re
hesitating to use Optional in data intensive operations.
For example, SingleInputGate is already creating Optional
for
every
BufferOrEvent in getNextBufferOrEvent(). How much
performance
gain
would
we
get if it’s replaced by null check?
Regards,
Qi
On Aug 1, 2019, at 11:00 PM, Andrey Zagrebin <
and...@ververica.com
wrote:
Hi all,
This is the next follow up discussion about suggestions
for
the
recent
thread about code style guide in Flink [1].
In general, one could argue that any variable, which is
nullable,
can
be
replaced by wrapping it with Optional to explicitly show
that
it
can
be
null. Examples are:
- returned values to force user to check not null
- optional function arguments, e.g. with implicit default
values
- even class fields as e.g. optional config options with
implicit
default values
At the same time, we also have @Nullable annotation to
express
this
intention.
Also, when the class Optional was introduced, Oracle
posted
a
guideline
about its usage [2]. Basically, it suggests to use it
mostly
in
APIs
for
returned values to inform and force users to check the
returned
value
instead of returning null and avoid NullPointerException.
Wrapping with Optional also comes with the performance
overhead.
Following the Oracle's guide in general, the suggestion
is:
- Avoid using Optional in any performance critical code
- Use Optional only to return nullable values in the
API/public
methods
unless it is performance critical then rather use
@Nullable
- Passing an Optional argument to a method can be allowed
if
it
is
within a private helper method and simplifies the code,
example
is
in
[3]
- Optional should not be used for class fields
Please, feel free to share you thoughts.
Best,
Andrey
[1]
http://mail-archives.apache.org/mod_mbox/flink-dev/201906.mbox/%3ced91df4b-7cab-4547-a430-85bc710fd...@apache.org%3E
[2]
https://www.oracle.com/technetwork/articles/java/java8-optional-2175753.html
[3]
https://github.com/apache/flink/blob/master/flink-formats/flink-avro/src/main/java/org/apache/flink/formats/avro/typeutils/AvroFactory.java#L95
