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
