[DISCUSS][Code-Style] The approach to implement singleton pattern

[Yangze Guo]

Hi, devs,

Recently, in the PR of FLINK-19179[1], we have a discussion about how
to implement singleton pattern in Flink.

Currently, most of the utility classes implement singleton pattern
through the private constructor. Seldom utility classes leverage the
enum mechanism. From my perspective, leveraging enum mechanism is more
simple and it can also overcome reflection.

Whether using enum classes or private constructors, it will be good to
align the approach to achieve singleton in the whole Flink project.

I would propose to leverage the enum mechanism in the Flink to
implement singleton pattern and append it to the code-style
guidelines. We may also have a JIRA ticket to refactor the existing
code.

What do you think?

[1] https://github.com/apache/flink/pull/13416

Best,
Yangze Guo

[Piotr Nowojski-5]

Hi,

I don't mind one way or the other.

I guess enum way is somehow safer, however did we really have any issues
with our current approach with `private` constructors? I mean, you are
mentioning that using reflections could overcome private constructors, but
is that a real concern in our code base? Has this caused some concrete
issues? If I would be reviewing a code doing things like this (or generally
speaking using reflections in the first place for anything), one should
better have a really good excuse :)

So all in all, I would be +1 for allowing `Enum`, -0.1 for banning
`private` constructors approach, but I also wouldn't mind sticking with
`private` constructors for the sake of consistency if the majority of the
community has strong feelings against using enums.

Piotrek

pt., 25 wrz 2020 o 10:22 Yangze Guo <[hidden email]> napisał(a):

> Hi, devs,
>
> Recently, in the PR of FLINK-19179[1], we have a discussion about how
> to implement singleton pattern in Flink.
>
> Currently, most of the utility classes implement singleton pattern
> through the private constructor. Seldom utility classes leverage the
> enum mechanism. From my perspective, leveraging enum mechanism is more
> simple and it can also overcome reflection.
>
> Whether using enum classes or private constructors, it will be good to
> align the approach to achieve singleton in the whole Flink project.
>
> I would propose to leverage the enum mechanism in the Flink to
> implement singleton pattern and append it to the code-style
> guidelines. We may also have a JIRA ticket to refactor the existing
> code.
>
> What do you think?
>
> [1] https://github.com/apache/flink/pull/13416
>
> Best,
> Yangze Guo
>

[Timo Walther-2]

Hi,

honstely, I find using enums is more of a hack. `enum` stands for
enumeration and is meant for listing flags or options. Using it for
singleton patterns is just abusing a concept due to certain
implementation details and less code.

I feel this topic is like using Lombok for generating hashCode/equals.
It means less coding but introduces another dependency and abuses the
annotation processor. I would rather use the Java language according to
the author's intention.

Regards,
Timo

On 25.09.20 11:22, Piotr Nowojski wrote:

> Hi,
>
> I don't mind one way or the other.
>
> I guess enum way is somehow safer, however did we really have any issues
> with our current approach with `private` constructors? I mean, you are
> mentioning that using reflections could overcome private constructors, but
> is that a real concern in our code base? Has this caused some concrete
> issues? If I would be reviewing a code doing things like this (or generally
> speaking using reflections in the first place for anything), one should
> better have a really good excuse :)
>
> So all in all, I would be +1 for allowing `Enum`, -0.1 for banning
> `private` constructors approach, but I also wouldn't mind sticking with
> `private` constructors for the sake of consistency if the majority of the
> community has strong feelings against using enums.
>
> Piotrek
>
> pt., 25 wrz 2020 o 10:22 Yangze Guo <[hidden email]> napisał(a):
>
>> Hi, devs,
>>
>> Recently, in the PR of FLINK-19179[1], we have a discussion about how
>> to implement singleton pattern in Flink.
>>
>> Currently, most of the utility classes implement singleton pattern
>> through the private constructor. Seldom utility classes leverage the
>> enum mechanism. From my perspective, leveraging enum mechanism is more
>> simple and it can also overcome reflection.
>>
>> Whether using enum classes or private constructors, it will be good to
>> align the approach to achieve singleton in the whole Flink project.
>>
>> I would propose to leverage the enum mechanism in the Flink to
>> implement singleton pattern and append it to the code-style
>> guidelines. We may also have a JIRA ticket to refactor the existing
>> code.
>>
>> What do you think?
>>
>> [1] https://github.com/apache/flink/pull/13416
>>
>> Best,
>> Yangze Guo
>>
>

[Gaël Renoux]

Hi

One small remark here: you should not call this a Singleton. For most
people, a Singleton would refer to the implementation of the GoF Singleton
pattern, where you have a single instance of the class (see for instance
the corresponding Wikipedia page:
https://en.wikipedia.org/wiki/Singleton_pattern#Java_Implementation_[7]).
Your discussion is about a non-instantiable class (containing only static
methods).

As for the choice between the two, the private constructor pattern seems
much more intuitive to me. If using enum, I think you need to impose some
documenting comment to explain the dangling semicolon (it's very weird to
coders unfamiliar with the pattern).

Gaël Renoux

On Fri, Sep 25, 2020 at 11:38 AM Timo Walther <[hidden email]> wrote:

> Hi,
>
> honstely, I find using enums is more of a hack. `enum` stands for
> enumeration and is meant for listing flags or options. Using it for
> singleton patterns is just abusing a concept due to certain
> implementation details and less code.
>
> I feel this topic is like using Lombok for generating hashCode/equals.
> It means less coding but introduces another dependency and abuses the
> annotation processor. I would rather use the Java language according to
> the author's intention.
>
> Regards,
> Timo
>
> On 25.09.20 11:22, Piotr Nowojski wrote:
> > Hi,
> >
> > I don't mind one way or the other.
> >
> > I guess enum way is somehow safer, however did we really have any issues
> > with our current approach with `private` constructors? I mean, you are
> > mentioning that using reflections could overcome private constructors,
> but
> > is that a real concern in our code base? Has this caused some concrete
> > issues? If I would be reviewing a code doing things like this (or
> generally
> > speaking using reflections in the first place for anything), one should
> > better have a really good excuse :)
> >
> > So all in all, I would be +1 for allowing `Enum`, -0.1 for banning
> > `private` constructors approach, but I also wouldn't mind sticking with
> > `private` constructors for the sake of consistency if the majority of the
> > community has strong feelings against using enums.
> >
> > Piotrek
> >
> > pt., 25 wrz 2020 o 10:22 Yangze Guo <[hidden email]> napisał(a):
> >
> >> Hi, devs,
> >>
> >> Recently, in the PR of FLINK-19179[1], we have a discussion about how
> >> to implement singleton pattern in Flink.
> >>
> >> Currently, most of the utility classes implement singleton pattern
> >> through the private constructor. Seldom utility classes leverage the
> >> enum mechanism. From my perspective, leveraging enum mechanism is more
> >> simple and it can also overcome reflection.
> >>
> >> Whether using enum classes or private constructors, it will be good to
> >> align the approach to achieve singleton in the whole Flink project.
> >>
> >> I would propose to leverage the enum mechanism in the Flink to
> >> implement singleton pattern and append it to the code-style
> >> guidelines. We may also have a JIRA ticket to refactor the existing
> >> code.
> >>
> >> What do you think?
> >>
> >> [1] https://github.com/apache/flink/pull/13416
> >>
> >> Best,
> >> Yangze Guo
> >>
> >
>
>

[dwysakowicz]

Hi all,

First of all I very much agree with Gael. The discussion is not about a
Singleton pattern.

Secondly, similarly as @Timo and @Gael I find the pattern very
confusing. Each time I see it I have a hard time figuring out why there
are no enumerations in the enum. This is my preference though.

Best,

Dawid

On 25/09/2020 11:42, Gaël Renoux wrote:

> Hi
>
> One small remark here: you should not call this a Singleton. For most
> people, a Singleton would refer to the implementation of the GoF Singleton
> pattern, where you have a single instance of the class (see for instance
> the corresponding Wikipedia page:
> https://en.wikipedia.org/wiki/Singleton_pattern#Java_Implementation_[7]).
> Your discussion is about a non-instantiable class (containing only static
> methods).
>
> As for the choice between the two, the private constructor pattern seems
> much more intuitive to me. If using enum, I think you need to impose some
> documenting comment to explain the dangling semicolon (it's very weird to
> coders unfamiliar with the pattern).
>
> Gaël Renoux
>
> On Fri, Sep 25, 2020 at 11:38 AM Timo Walther <[hidden email]> wrote:
>
>> Hi,
>>
>> honstely, I find using enums is more of a hack. `enum` stands for
>> enumeration and is meant for listing flags or options. Using it for
>> singleton patterns is just abusing a concept due to certain
>> implementation details and less code.
>>
>> I feel this topic is like using Lombok for generating hashCode/equals.
>> It means less coding but introduces another dependency and abuses the
>> annotation processor. I would rather use the Java language according to
>> the author's intention.
>>
>> Regards,
>> Timo
>>
>> On 25.09.20 11:22, Piotr Nowojski wrote:
>>> Hi,
>>>
>>> I don't mind one way or the other.
>>>
>>> I guess enum way is somehow safer, however did we really have any issues
>>> with our current approach with `private` constructors? I mean, you are
>>> mentioning that using reflections could overcome private constructors,
>> but
>>> is that a real concern in our code base? Has this caused some concrete
>>> issues? If I would be reviewing a code doing things like this (or
>> generally
>>> speaking using reflections in the first place for anything), one should
>>> better have a really good excuse :)
>>>
>>> So all in all, I would be +1 for allowing `Enum`, -0.1 for banning
>>> `private` constructors approach, but I also wouldn't mind sticking with
>>> `private` constructors for the sake of consistency if the majority of the
>>> community has strong feelings against using enums.
>>>
>>> Piotrek
>>>
>>> pt., 25 wrz 2020 o 10:22 Yangze Guo <[hidden email]> napisał(a):
>>>
>>>> Hi, devs,
>>>>
>>>> Recently, in the PR of FLINK-19179[1], we have a discussion about how
>>>> to implement singleton pattern in Flink.
>>>>
>>>> Currently, most of the utility classes implement singleton pattern
>>>> through the private constructor. Seldom utility classes leverage the
>>>> enum mechanism. From my perspective, leveraging enum mechanism is more
>>>> simple and it can also overcome reflection.
>>>>
>>>> Whether using enum classes or private constructors, it will be good to
>>>> align the approach to achieve singleton in the whole Flink project.
>>>>
>>>> I would propose to leverage the enum mechanism in the Flink to
>>>> implement singleton pattern and append it to the code-style
>>>> guidelines. We may also have a JIRA ticket to refactor the existing
>>>> code.
>>>>
>>>> What do you think?
>>>>
>>>> [1] https://github.com/apache/flink/pull/13416
>>>>
>>>> Best,
>>>> Yangze Guo
>>>>
>>

[Jingsong Li]

Hi, thanks for starting this discussion.

I am +1 for using the private constructor for util class. We don't need to
change it.

I think few libraries use the enum, such as guava, common-utils, or even
JDK, the private constructor is widely used.

I don't quite understand why a util class is an enum. Enum seems to be
conceptually different from general classes. It's just a util class, and I
don't want to enumerate it.

And I think what Timo said makes sense to me.

Best,
Jingsong

On Fri, Sep 25, 2020 at 5:38 PM Timo Walther <[hidden email]> wrote:

> Hi,
>
> honstely, I find using enums is more of a hack. `enum` stands for
> enumeration and is meant for listing flags or options. Using it for
> singleton patterns is just abusing a concept due to certain
> implementation details and less code.
>
> I feel this topic is like using Lombok for generating hashCode/equals.
> It means less coding but introduces another dependency and abuses the
> annotation processor. I would rather use the Java language according to
> the author's intention.
>
> Regards,
> Timo
>
> On 25.09.20 11:22, Piotr Nowojski wrote:
> > Hi,
> >
> > I don't mind one way or the other.
> >
> > I guess enum way is somehow safer, however did we really have any issues
> > with our current approach with `private` constructors? I mean, you are
> > mentioning that using reflections could overcome private constructors,
> but
> > is that a real concern in our code base? Has this caused some concrete
> > issues? If I would be reviewing a code doing things like this (or
> generally
> > speaking using reflections in the first place for anything), one should
> > better have a really good excuse :)
> >
> > So all in all, I would be +1 for allowing `Enum`, -0.1 for banning
> > `private` constructors approach, but I also wouldn't mind sticking with
> > `private` constructors for the sake of consistency if the majority of the
> > community has strong feelings against using enums.
> >
> > Piotrek
> >
> > pt., 25 wrz 2020 o 10:22 Yangze Guo <[hidden email]> napisał(a):
> >
> >> Hi, devs,
> >>
> >> Recently, in the PR of FLINK-19179[1], we have a discussion about how
> >> to implement singleton pattern in Flink.
> >>
> >> Currently, most of the utility classes implement singleton pattern
> >> through the private constructor. Seldom utility classes leverage the
> >> enum mechanism. From my perspective, leveraging enum mechanism is more
> >> simple and it can also overcome reflection.
> >>
> >> Whether using enum classes or private constructors, it will be good to
> >> align the approach to achieve singleton in the whole Flink project.
> >>
> >> I would propose to leverage the enum mechanism in the Flink to
> >> implement singleton pattern and append it to the code-style
> >> guidelines. We may also have a JIRA ticket to refactor the existing
> >> code.
> >>
> >> What do you think?
> >>
> >> [1] https://github.com/apache/flink/pull/13416
> >>
> >> Best,
> >> Yangze Guo
> >>
> >
>
>

--
Best, Jingsong Lee

[Yangze Guo]

Thanks all for the valuable feedbacks!

@Gael @Dawid
Thanks for the explanation! I think you are right that this discussion
is about a non-instantiable class that contains only static methods.

@All
My major proposal is actually to stick to one of two approaches in
Flink. It seems that most devs prefer private constructor. And to be
honest, I also could not see any drawbacks to that approach. All in
all, we may choose the private constructor approach instead.

If you think we should ban one of these two approaches, please +1 and
tell your preferences.
If you think we should not enforce it, please -1.

Thanks for all the feedbacks again. Looking forward to your comments!

Best,
Yangze Guo

On Fri, Sep 25, 2020 at 5:50 PM Jingsong Li <[hidden email]> wrote:

>
> Hi, thanks for starting this discussion.
>
> I am +1 for using the private constructor for util class. We don't need to
> change it.
>
> I think few libraries use the enum, such as guava, common-utils, or even
> JDK, the private constructor is widely used.
>
> I don't quite understand why a util class is an enum. Enum seems to be
> conceptually different from general classes. It's just a util class, and I
> don't want to enumerate it.
>
> And I think what Timo said makes sense to me.
>
> Best,
> Jingsong
>
> On Fri, Sep 25, 2020 at 5:38 PM Timo Walther <[hidden email]> wrote:
>
> > Hi,
> >
> > honstely, I find using enums is more of a hack. `enum` stands for
> > enumeration and is meant for listing flags or options. Using it for
> > singleton patterns is just abusing a concept due to certain
> > implementation details and less code.
> >
> > I feel this topic is like using Lombok for generating hashCode/equals.
> > It means less coding but introduces another dependency and abuses the
> > annotation processor. I would rather use the Java language according to
> > the author's intention.
> >
> > Regards,
> > Timo
> >
> > On 25.09.20 11:22, Piotr Nowojski wrote:
> > > Hi,
> > >
> > > I don't mind one way or the other.
> > >
> > > I guess enum way is somehow safer, however did we really have any issues
> > > with our current approach with `private` constructors? I mean, you are
> > > mentioning that using reflections could overcome private constructors,
> > but
> > > is that a real concern in our code base? Has this caused some concrete
> > > issues? If I would be reviewing a code doing things like this (or
> > generally
> > > speaking using reflections in the first place for anything), one should
> > > better have a really good excuse :)
> > >
> > > So all in all, I would be +1 for allowing `Enum`, -0.1 for banning
> > > `private` constructors approach, but I also wouldn't mind sticking with
> > > `private` constructors for the sake of consistency if the majority of the
> > > community has strong feelings against using enums.
> > >
> > > Piotrek
> > >
> > > pt., 25 wrz 2020 o 10:22 Yangze Guo <[hidden email]> napisał(a):
> > >
> > >> Hi, devs,
> > >>
> > >> Recently, in the PR of FLINK-19179[1], we have a discussion about how
> > >> to implement singleton pattern in Flink.
> > >>
> > >> Currently, most of the utility classes implement singleton pattern
> > >> through the private constructor. Seldom utility classes leverage the
> > >> enum mechanism. From my perspective, leveraging enum mechanism is more
> > >> simple and it can also overcome reflection.
> > >>
> > >> Whether using enum classes or private constructors, it will be good to
> > >> align the approach to achieve singleton in the whole Flink project.
> > >>
> > >> I would propose to leverage the enum mechanism in the Flink to
> > >> implement singleton pattern and append it to the code-style
> > >> guidelines. We may also have a JIRA ticket to refactor the existing
> > >> code.
> > >>
> > >> What do you think?
> > >>
> > >> [1] https://github.com/apache/flink/pull/13416
> > >>
> > >> Best,
> > >> Yangze Guo
> > >>
> > >
> >
> >
>
> --
> Best, Jingsong Lee

[Xintong Song]

Thanks for starting this discussion, Yangze.


My personal preference for either singleton or non-initiatable classes is
to use enum wherever it is possible, because it's briefer is safer. On the
other hand, I'm also against private constructors.


To my understanding, for most if not all suggestions in the code style
guidelines, there are good reasons supporting the suggestions (e.g., there
might be bad consequences not following the suggestions).


For this case, I think neither of the 2 approaches does any serious harm.
It's more about personal preference. I don't see the necessity to force one
over the other.


Therefore, I would be -1 for banning one of them.


Thank you~

Xintong Song




On Fri, Sep 25, 2020 at 6:46 PM Yangze Guo <[hidden email]> wrote:

> Thanks all for the valuable feedbacks!
>
> @Gael @Dawid
> Thanks for the explanation! I think you are right that this discussion
> is about a non-instantiable class that contains only static methods.
>
> @All
> My major proposal is actually to stick to one of two approaches in
> Flink. It seems that most devs prefer private constructor. And to be
> honest, I also could not see any drawbacks to that approach. All in
> all, we may choose the private constructor approach instead.
>
> If you think we should ban one of these two approaches, please +1 and
> tell your preferences.
> If you think we should not enforce it, please -1.
>
> Thanks for all the feedbacks again. Looking forward to your comments!
>
> Best,
> Yangze Guo
>
> On Fri, Sep 25, 2020 at 5:50 PM Jingsong Li <[hidden email]>
> wrote:
> >
> > Hi, thanks for starting this discussion.
> >
> > I am +1 for using the private constructor for util class. We don't need
> to
> > change it.
> >
> > I think few libraries use the enum, such as guava, common-utils, or even
> > JDK, the private constructor is widely used.
> >
> > I don't quite understand why a util class is an enum. Enum seems to be
> > conceptually different from general classes. It's just a util class, and
> I
> > don't want to enumerate it.
> >
> > And I think what Timo said makes sense to me.
> >
> > Best,
> > Jingsong
> >
> > On Fri, Sep 25, 2020 at 5:38 PM Timo Walther <[hidden email]> wrote:
> >
> > > Hi,
> > >
> > > honstely, I find using enums is more of a hack. `enum` stands for
> > > enumeration and is meant for listing flags or options. Using it for
> > > singleton patterns is just abusing a concept due to certain
> > > implementation details and less code.
> > >
> > > I feel this topic is like using Lombok for generating hashCode/equals.
> > > It means less coding but introduces another dependency and abuses the
> > > annotation processor. I would rather use the Java language according to
> > > the author's intention.
> > >
> > > Regards,
> > > Timo
> > >
> > > On 25.09.20 11:22, Piotr Nowojski wrote:
> > > > Hi,
> > > >
> > > > I don't mind one way or the other.
> > > >
> > > > I guess enum way is somehow safer, however did we really have any
> issues
> > > > with our current approach with `private` constructors? I mean, you
> are
> > > > mentioning that using reflections could overcome private
> constructors,
> > > but
> > > > is that a real concern in our code base? Has this caused some
> concrete
> > > > issues? If I would be reviewing a code doing things like this (or
> > > generally
> > > > speaking using reflections in the first place for anything), one
> should
> > > > better have a really good excuse :)
> > > >
> > > > So all in all, I would be +1 for allowing `Enum`, -0.1 for banning
> > > > `private` constructors approach, but I also wouldn't mind sticking
> with
> > > > `private` constructors for the sake of consistency if the majority
> of the
> > > > community has strong feelings against using enums.
> > > >
> > > > Piotrek
> > > >
> > > > pt., 25 wrz 2020 o 10:22 Yangze Guo <[hidden email]> napisał(a):
> > > >
> > > >> Hi, devs,
> > > >>
> > > >> Recently, in the PR of FLINK-19179[1], we have a discussion about
> how
> > > >> to implement singleton pattern in Flink.
> > > >>
> > > >> Currently, most of the utility classes implement singleton pattern
> > > >> through the private constructor. Seldom utility classes leverage the
> > > >> enum mechanism. From my perspective, leveraging enum mechanism is
> more
> > > >> simple and it can also overcome reflection.
> > > >>
> > > >> Whether using enum classes or private constructors, it will be good
> to
> > > >> align the approach to achieve singleton in the whole Flink project.
> > > >>
> > > >> I would propose to leverage the enum mechanism in the Flink to
> > > >> implement singleton pattern and append it to the code-style
> > > >> guidelines. We may also have a JIRA ticket to refactor the existing
> > > >> code.
> > > >>
> > > >> What do you think?
> > > >>
> > > >> [1] https://github.com/apache/flink/pull/13416
> > > >>
> > > >> Best,
> > > >> Yangze Guo
> > > >>
> > > >
> > >
> > >
> >
> > --
> > Best, Jingsong Lee
>