@ProcessElement and private methods

[Stas Levin <st...@gmail.com>]

Hi,

At the moment only public methods are eligible to be decorated with
@ProcessElement (DoFnSignatures#findAnnotatedMethod).

Seems that from a technical point of view, it would be quite possible to
consider private methods as well. In fact, it's one of the benefits of
moving towards an annotation based discovery as opposed to an interface
based one.

Do you think being able to decorate non-public methods with @ProcessElement
(and similar annotations) is something we'd like to support?

Regards,
Stas

Re: @ProcessElement and private methods

[Jean-Baptiste Onofré <jb...@nanthrax.net>]

Hi Stas

Just curious: what would be the use case of private method ? Being able to reuse the DoFn class in different contexts ?

Regards
JB\u2063\u200b

On Jan 17, 2017, 07:33, at 07:33, Stas Levin <st...@gmail.com> wrote:
>Hi,
>
>At the moment only public methods are eligible to be decorated with
>@ProcessElement (DoFnSignatures#findAnnotatedMethod).
>
>Seems that from a technical point of view, it would be quite possible
>to
>consider private methods as well. In fact, it's one of the benefits of
>moving towards an annotation based discovery as opposed to an interface
>based one.
>
>Do you think being able to decorate non-public methods with
>@ProcessElement
>(and similar annotations) is something we'd like to support?
>
>Regards,
>Stas

Re: @ProcessElement and private methods

[Aljoscha Krettek <al...@apache.org>]

I think the actual implementation is another argument against allowing
private methods. I'm not super familiar with it but we actually generate
code for calling the annotated methods and that code has to adhere to Java
restrictions and cannot call private methods. I think calling private
methods is only possible using reflection, which would be a lot slower.

I could be completely wrong, this is just off the top of my hat.

On Tue, 17 Jan 2017 at 20:47 Lukasz Cwik <lc...@google.com.invalid> wrote:

> Requiring public is also a form of documentation for implementers and
> maintainers that this method is meant to be called by someone else.
>
> All our arguments have been around best practices.
>
> I think the only technical argument is that if a runner today chooses to
> execute with a security manager, it may disallow making private methods
> accessible. From my knowledge, none of the runners currently use a security
> manager so its a weak point.
>
> On Tue, Jan 17, 2017 at 10:07 AM, Stas Levin <st...@gmail.com> wrote:
>
> > We stumbled across this issue when we were doing some Scala-Java interop
> > and had an anonymous class that was supposed to have an element
> processing
> > method. The Scala compiler had decided to make that method private, and
> so
> > it was not visible to Beam. While extracting this class to an upper level
> > (i.e., non-anonymous) solved the problem, it made me wonder.
> >
> > I can see the "overrides-in-spirit" point, but since it's not really an
> > interface in the regular sense (e.g., no polymorphism) I can also imagine
> > cases where users may want to encapsulate (i.e. make private) the element
> > processing logic inside a class. It's already a reflection based
> mechanism
> > so the difference between inspecting public only vs. all may not be that
> > big, and as far as users are concerned both are "magic" :)
> >
> > IMHO, the same argument could apply to JUnit, and as a JUnit user, I'm
> not
> > sure I can see the benefit of forcing test methods and rules to be public
> > in the scope of a test class. Some would even say this may not be ideal
> in
> > terms of encapsulation. I might be missing something here, so I wanted to
> > get some more perspective.
> >
> > On Tue, Jan 17, 2017 at 6:47 PM Ben Chambers
> <bchambers@google.com.invalid
> > >
> > wrote:
> >
> > > We thought about it when this was added. We decided against it because
> > > these are overrides-in-spirit. Letting them be private would be
> > misleading
> > > because they are called from outside the class and should be thought of
> > in
> > > that way.
> > >
> > > Also, this seems similar to others in the Java ecosystem: JUnit test
> > > methods are public as well.
> > >
> > > What is the motivation for allowing private methods?
> > >
> > >
> > > On Tue, Jan 17, 2017, 7:33 AM Stas Levin <st...@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > At the moment only public methods are eligible to be decorated with
> > > @ProcessElement (DoFnSignatures#findAnnotatedMethod).
> > >
> > > Seems that from a technical point of view, it would be quite possible
> to
> > > consider private methods as well. In fact, it's one of the benefits of
> > > moving towards an annotation based discovery as opposed to an interface
> > > based one.
> > >
> > > Do you think being able to decorate non-public methods with
> > @ProcessElement
> > > (and similar annotations) is something we'd like to support?
> > >
> > > Regards,
> > > Stas
> > >
> >
>

Re: @ProcessElement and private methods

[Lukasz Cwik <lc...@google.com.INVALID>]

Requiring public is also a form of documentation for implementers and
maintainers that this method is meant to be called by someone else.

All our arguments have been around best practices.

I think the only technical argument is that if a runner today chooses to
execute with a security manager, it may disallow making private methods
accessible. From my knowledge, none of the runners currently use a security
manager so its a weak point.

On Tue, Jan 17, 2017 at 10:07 AM, Stas Levin <st...@gmail.com> wrote:

> We stumbled across this issue when we were doing some Scala-Java interop
> and had an anonymous class that was supposed to have an element processing
> method. The Scala compiler had decided to make that method private, and so
> it was not visible to Beam. While extracting this class to an upper level
> (i.e., non-anonymous) solved the problem, it made me wonder.
>
> I can see the "overrides-in-spirit" point, but since it's not really an
> interface in the regular sense (e.g., no polymorphism) I can also imagine
> cases where users may want to encapsulate (i.e. make private) the element
> processing logic inside a class. It's already a reflection based mechanism
> so the difference between inspecting public only vs. all may not be that
> big, and as far as users are concerned both are "magic" :)
>
> IMHO, the same argument could apply to JUnit, and as a JUnit user, I'm not
> sure I can see the benefit of forcing test methods and rules to be public
> in the scope of a test class. Some would even say this may not be ideal in
> terms of encapsulation. I might be missing something here, so I wanted to
> get some more perspective.
>
> On Tue, Jan 17, 2017 at 6:47 PM Ben Chambers <bchambers@google.com.invalid
> >
> wrote:
>
> > We thought about it when this was added. We decided against it because
> > these are overrides-in-spirit. Letting them be private would be
> misleading
> > because they are called from outside the class and should be thought of
> in
> > that way.
> >
> > Also, this seems similar to others in the Java ecosystem: JUnit test
> > methods are public as well.
> >
> > What is the motivation for allowing private methods?
> >
> >
> > On Tue, Jan 17, 2017, 7:33 AM Stas Levin <st...@gmail.com> wrote:
> >
> > Hi,
> >
> > At the moment only public methods are eligible to be decorated with
> > @ProcessElement (DoFnSignatures#findAnnotatedMethod).
> >
> > Seems that from a technical point of view, it would be quite possible to
> > consider private methods as well. In fact, it's one of the benefits of
> > moving towards an annotation based discovery as opposed to an interface
> > based one.
> >
> > Do you think being able to decorate non-public methods with
> @ProcessElement
> > (and similar annotations) is something we'd like to support?
> >
> > Regards,
> > Stas
> >
>

Re: @ProcessElement and private methods

[Stas Levin <st...@gmail.com>]

We stumbled across this issue when we were doing some Scala-Java interop
and had an anonymous class that was supposed to have an element processing
method. The Scala compiler had decided to make that method private, and so
it was not visible to Beam. While extracting this class to an upper level
(i.e., non-anonymous) solved the problem, it made me wonder.

I can see the "overrides-in-spirit" point, but since it's not really an
interface in the regular sense (e.g., no polymorphism) I can also imagine
cases where users may want to encapsulate (i.e. make private) the element
processing logic inside a class. It's already a reflection based mechanism
so the difference between inspecting public only vs. all may not be that
big, and as far as users are concerned both are "magic" :)

IMHO, the same argument could apply to JUnit, and as a JUnit user, I'm not
sure I can see the benefit of forcing test methods and rules to be public
in the scope of a test class. Some would even say this may not be ideal in
terms of encapsulation. I might be missing something here, so I wanted to
get some more perspective.

On Tue, Jan 17, 2017 at 6:47 PM Ben Chambers <bc...@google.com.invalid>
wrote:

> We thought about it when this was added. We decided against it because
> these are overrides-in-spirit. Letting them be private would be misleading
> because they are called from outside the class and should be thought of in
> that way.
>
> Also, this seems similar to others in the Java ecosystem: JUnit test
> methods are public as well.
>
> What is the motivation for allowing private methods?
>
>
> On Tue, Jan 17, 2017, 7:33 AM Stas Levin <st...@gmail.com> wrote:
>
> Hi,
>
> At the moment only public methods are eligible to be decorated with
> @ProcessElement (DoFnSignatures#findAnnotatedMethod).
>
> Seems that from a technical point of view, it would be quite possible to
> consider private methods as well. In fact, it's one of the benefits of
> moving towards an annotation based discovery as opposed to an interface
> based one.
>
> Do you think being able to decorate non-public methods with @ProcessElement
> (and similar annotations) is something we'd like to support?
>
> Regards,
> Stas
>

Re: @ProcessElement and private methods

[Ben Chambers <bc...@google.com.INVALID>]

We thought about it when this was added. We decided against it because
these are overrides-in-spirit. Letting them be private would be misleading
because they are called from outside the class and should be thought of in
that way.

Also, this seems similar to others in the Java ecosystem: JUnit test
methods are public as well.

What is the motivation for allowing private methods?


On Tue, Jan 17, 2017, 7:33 AM Stas Levin <st...@gmail.com> wrote:

Hi,

At the moment only public methods are eligible to be decorated with
@ProcessElement (DoFnSignatures#findAnnotatedMethod).

Seems that from a technical point of view, it would be quite possible to
consider private methods as well. In fact, it's one of the benefits of
moving towards an annotation based discovery as opposed to an interface
based one.

Do you think being able to decorate non-public methods with @ProcessElement
(and similar annotations) is something we'd like to support?

Regards,
Stas