(DEPRECATED) Apache Flink Mailing List archive.

[DISCUSS][CODE STYLE] Usage of Java Optional

Classic

List

Threaded

24 messages Options

Andrey Zagrebin-3

[DISCUSS][CODE STYLE] Usage of Java Optional

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

Re: [DISCUSS][CODE STYLE] Usage of Java Optional

EDIT: for Optional in public API vs performance concerns

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:

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

On Thu, Aug 1, 2019 at 6: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
>

Piotr Nowojski-3

Re: [DISCUSS][CODE STYLE] Usage of Java Optional

Hi Andrey,

I think that’s a good compromise easy to follow, so +1 from my side.

Piotrek

> On 1 Aug 2019, at 18:00, Andrey Zagrebin <[hidden email]> wrote:
>
> EDIT: for Optional in public API vs performance concerns
>
> 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:
>
> - 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, 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
>
> On Thu, Aug 1, 2019 at 6: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
>>

qi luo

Re: [DISCUSS][CODE STYLE] Usage of Java Optional

In reply to this post by Andrey Zagrebin-3

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

Biao Liu

Re: [DISCUSS][CODE STYLE] Usage of Java Optional

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

Jark Wu-2

Re: [DISCUSS][CODE STYLE] Usage of Java Optional

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

Re: [DISCUSS][CODE STYLE] Usage of Java Optional

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

Jark Wu-2

Re: [DISCUSS][CODE STYLE] Usage of Java Optional

Hi Zili,

Yes. I agree to use @Nullable/@Nonnull/SerializableOptional as the class
field instead of Optional.

On Fri, 2 Aug 2019 at 17:00, 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
> > > >
> > > >
> > >
> >
>

Biao Liu

Re: [DISCUSS][CODE STYLE] Usage of Java Optional

In reply to this post by tison

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:

Timo Walther-2

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

JingsongLee-2

Re: [DISCUSS][CODE STYLE] Usage of Java Optional

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:

Yu Li

Re: [DISCUSS][CODE STYLE] Usage of Java Optional

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

Aljoscha Krettek-2

Re: [DISCUSS][CODE STYLE] Usage of Java Optional

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

Till Rohrmann

Re: [DISCUSS][CODE STYLE] Usage of Java Optional

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

Piotr Nowojski-3

Re: [DISCUSS][CODE STYLE] Usage of Java Optional

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

Re: [DISCUSS][CODE STYLE] Usage of Java Optional

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

Stephan Ewen

Re: [DISCUSS][CODE STYLE] Usage of Java Optional

For the use of optional in private methods: It sounds fine to me, because
it means it is strictly class-internal (between methods and helper methods)
and does not leak beyond that.

On Mon, Aug 19, 2019 at 5:53 PM Andrey Zagrebin <[hidden email]>
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
> > >>>>>>>>>
> > >>>>
> > >>>>
> > >>>>
> > >>
> > >>
> >
> >
>

dwysakowicz

Re: [DISCUSS][CODE STYLE] Usage of Java Optional

In reply to this post by Andrey Zagrebin-3

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

signature.asc (849 bytes) Download Attachment

Stephan Ewen

Re: [DISCUSS][CODE STYLE] Usage of Java Optional

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

Yu Li

Re: [DISCUSS][CODE STYLE] Usage of Java Optional

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