Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
I've restored the previous method signature to LoadIncrementalHFiles, so
that piece is complete for the next RC.
Based on the latest comments, let me update my proposed course of action.
1. restore any VisibleForTesting method signatures for 2.3.0, treat this as
public API going forward.
2. purge the VisibleForTesting annotation from our codebase for 2.4+,
involving:
2a. replace VisibleForTesting with IA.Private anywhere method visibility
cannot be limited
2b. perhaps add a new Yetus check that would ban new use of
VisibleForTesting
I have filed https://issues.apache.org/jira/browse/HBASE-24640 as a
tracking task for this work. If the above process is agreeable, let's add
these steps as a comment for future reference.
Thanks,
Nick
On Fri, Jun 26, 2020 at 8:38 AM 张铎(Duo Zhang) wrote:
> For the LoadIncrementalHFiles class, the IA.Public annotation itself is a
> problem. It should be IA.LimitPrivate(TOOLS). So I'm fine with either
> adding the method back or not, the opinion of the release manager is
> most important I think.
>
> Thanks.
>
> Viraj Jasani 于2020年6月26日周五 下午9:50写道：
>
> > Agree on replacing VFT in next 2.4.0 or 3.0.0 release and restoring the
> > required
> > method for now to unblock 2.3.0 RC1.
> >
> >
> > On 2020/06/26 01:11:31, Andrew Purtell wrote:
> > > Sounds fine to me.
> > >
> > > My earlier objection was to talk of an HBase 3 followed by an HBase 4.
> We
> > > don't need to do a full deprecation cycle across two major versions to
> > > remove an annotation that never promised public access. (By definition,
> > > tagged fields and members were VisibleForTesting (only). The 'only' was
> > > implied, but I think a reasonable assumption and common knowledge.)
> > >
> > > On Thu, Jun 25, 2020 at 3:48 PM Sean Busbey wrote:
> > >
> > > > Agree on restoring the member and then getting this done for 2.4.0.
> > > >
> > > >
> > > > On Thu, Jun 25, 2020, 15:02 Nick Dimiduk
> wrote:
> > > >
> > > > > And now by module,
> > > > >
> > > > > $ find . -iname '*.java' -exec grep -n '@VisibleForTesting' {} \+ |
> > cut
> > > > -d/
> > > > > -f2 | sort | uniq -c
> > > > >6 hbase-backup
> > > > > 87 hbase-client
> > > > > 40 hbase-common
> > > > >1 hbase-endpoint
> > > > >7 hbase-hadoop-compat
> > > > >3 hbase-http
> > > > > 18 hbase-mapreduce
> > > > >1 hbase-metrics-api
> > > > > 24 hbase-procedure
> > > > > 10 hbase-replication
> > > > > 456 hbase-server
> > > > >2 hbase-thrift
> > > > >1 hbase-zookeeper
> > > > >
> > > > > I prefer we not make this change a prerequisite to 2.3. I would
> > rather we
> > > > > restore the one method modified by HBASE-24221 and do the work for
> > > > > VisibleForTesting for 2.4.0.
> > > > >
> > > > > On Thu, Jun 25, 2020 at 12:57 PM Nick Dimiduk >
> > > > wrote:
> > > > >
> > > > > > On Thu, Jun 25, 2020 at 12:36 PM Andrew Purtell <
> > apurt...@apache.org>
> > > > > > wrote:
> > > > > >
> > > > > >> I think we are in agreement except for a need to have a
> > deprecation
> > > > > cycle.
> > > > > >> Just remove VisibleForTesting and replace with whatever
> > alternative
> > > > you
> > > > > >> like. Certainly in the next minors. No strong opinion either way
> > about
> > > > > >> patch releases, leave as is?
> > > > > >>
> > > > > >
> > > > > > Thanks Andrew and Bharath, I now better understand your
> positions.
> > > > > >
> > > > > > The annotation is fairly common in our codebase, from branch-2.3,
> > > > > >
> > > > > > $ find . -iname '*.java' -exec grep -n '@VisibleForTesting' {} \+
> > | wc
> > > > -l
> > > > > > 668
> > > > > >
> > > > > > I don't have an easy way to cross-reference this with our AI
> > > > annotations,
> > > > > > but my concern is that any change we make here without a
> > deprecation
> > > > > cycle
> > > > > > will be disruptive to users.
> > > > > >
> > > > > > On Thu, Jun 25, 2020 at 11:30 AM Nick Dimiduk <
> ndimi...@apache.org
> > >
> > > > > wrote:
> > > > > >>
> > > > > >> > On Wed, Jun 24, 2020 at 3:19 PM Andrew Purtell <
> > apurt...@apache.org
> > > > >
> > > > > >> > wrote:
> > > > > >> >
> > > > > >> > > It is possible some users may not understand what Guava's
> > > > > >> > VisibleForTesting
> > > > > >> > > implies, but those users are much more likely to be Java
> > > > developers
> > > > > or
> > > > > >> > Java
> > > > > >> > > developer adjacent, and familiar with what this fad
> entailed.
> > Such
> > > > > >> > tagging
> > > > > >> > > was/is done specifically to indicate the exposed field or
> > method
> > > > was
> > > > > >> only
> > > > > >> > > made to allow test access to internals, as something less
> than
> > > > > public.
> > > > > >> > >
> > > > > >> > > For us to treat such annotated fields and methods as public
> > after
> > > > > all
> > > > > >> is
> > > > > >> > > unnecessary, possibly surprising, and not semantically sound
> > > > (IMHO).
> > > > > >> > >
> > > > > >> >
> > > > > >> > I don't want to preserve use of VisibleForTesting as an
> >
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
For the LoadIncrementalHFiles class, the IA.Public annotation itself is a
problem. It should be IA.LimitPrivate(TOOLS). So I'm fine with either
adding the method back or not, the opinion of the release manager is
most important I think.
Thanks.
Viraj Jasani 于2020年6月26日周五 下午9:50写道：
> Agree on replacing VFT in next 2.4.0 or 3.0.0 release and restoring the
> required
> method for now to unblock 2.3.0 RC1.
>
>
> On 2020/06/26 01:11:31, Andrew Purtell wrote:
> > Sounds fine to me.
> >
> > My earlier objection was to talk of an HBase 3 followed by an HBase 4. We
> > don't need to do a full deprecation cycle across two major versions to
> > remove an annotation that never promised public access. (By definition,
> > tagged fields and members were VisibleForTesting (only). The 'only' was
> > implied, but I think a reasonable assumption and common knowledge.)
> >
> > On Thu, Jun 25, 2020 at 3:48 PM Sean Busbey wrote:
> >
> > > Agree on restoring the member and then getting this done for 2.4.0.
> > >
> > >
> > > On Thu, Jun 25, 2020, 15:02 Nick Dimiduk wrote:
> > >
> > > > And now by module,
> > > >
> > > > $ find . -iname '*.java' -exec grep -n '@VisibleForTesting' {} \+ |
> cut
> > > -d/
> > > > -f2 | sort | uniq -c
> > > >6 hbase-backup
> > > > 87 hbase-client
> > > > 40 hbase-common
> > > >1 hbase-endpoint
> > > >7 hbase-hadoop-compat
> > > >3 hbase-http
> > > > 18 hbase-mapreduce
> > > >1 hbase-metrics-api
> > > > 24 hbase-procedure
> > > > 10 hbase-replication
> > > > 456 hbase-server
> > > >2 hbase-thrift
> > > >1 hbase-zookeeper
> > > >
> > > > I prefer we not make this change a prerequisite to 2.3. I would
> rather we
> > > > restore the one method modified by HBASE-24221 and do the work for
> > > > VisibleForTesting for 2.4.0.
> > > >
> > > > On Thu, Jun 25, 2020 at 12:57 PM Nick Dimiduk
> > > wrote:
> > > >
> > > > > On Thu, Jun 25, 2020 at 12:36 PM Andrew Purtell <
> apurt...@apache.org>
> > > > > wrote:
> > > > >
> > > > >> I think we are in agreement except for a need to have a
> deprecation
> > > > cycle.
> > > > >> Just remove VisibleForTesting and replace with whatever
> alternative
> > > you
> > > > >> like. Certainly in the next minors. No strong opinion either way
> about
> > > > >> patch releases, leave as is?
> > > > >>
> > > > >
> > > > > Thanks Andrew and Bharath, I now better understand your positions.
> > > > >
> > > > > The annotation is fairly common in our codebase, from branch-2.3,
> > > > >
> > > > > $ find . -iname '*.java' -exec grep -n '@VisibleForTesting' {} \+
> | wc
> > > -l
> > > > > 668
> > > > >
> > > > > I don't have an easy way to cross-reference this with our AI
> > > annotations,
> > > > > but my concern is that any change we make here without a
> deprecation
> > > > cycle
> > > > > will be disruptive to users.
> > > > >
> > > > > On Thu, Jun 25, 2020 at 11:30 AM Nick Dimiduk >
> > > > wrote:
> > > > >>
> > > > >> > On Wed, Jun 24, 2020 at 3:19 PM Andrew Purtell <
> apurt...@apache.org
> > > >
> > > > >> > wrote:
> > > > >> >
> > > > >> > > It is possible some users may not understand what Guava's
> > > > >> > VisibleForTesting
> > > > >> > > implies, but those users are much more likely to be Java
> > > developers
> > > > or
> > > > >> > Java
> > > > >> > > developer adjacent, and familiar with what this fad entailed.
> Such
> > > > >> > tagging
> > > > >> > > was/is done specifically to indicate the exposed field or
> method
> > > was
> > > > >> only
> > > > >> > > made to allow test access to internals, as something less than
> > > > public.
> > > > >> > >
> > > > >> > > For us to treat such annotated fields and methods as public
> after
> > > > all
> > > > >> is
> > > > >> > > unnecessary, possibly surprising, and not semantically sound
> > > (IMHO).
> > > > >> > >
> > > > >> >
> > > > >> > I don't want to preserve use of VisibleForTesting as an
> indicator of
> > > > >> public
> > > > >> > API. I want to ensure that we're clear to our downstream users
> > > > >> > that its presence is not a factor in determining public API. For
> > > > >> example, I
> > > > >> > don't want to update our book to give any meaning to this
> > > annotation,
> > > > >> and I
> > > > >> > don't want to update our javadoc filters to take it into account
> > > when
> > > > >> > generating the various versions of javadoc that we publish. I
> want
> > > to
> > > > >> purge
> > > > >> > it from the discussion by annotating the methods it decorates
> with
> > > the
> > > > >> > symbols we do use to define our public API. The steps I propose
> > > above
> > > > >> are
> > > > >> > my suggestion of how we work toward that goal.
> > > > >> >
> > > > >> > Does anyone have a counter-proposal to the steps I've outlined
> > > above?
> > > > A
> > > > >> > resolution to this discussion is now the final blocker on
> 2.3.0rc1.
> > > > >> >
> > > > >> > Thanks,
> > > > >> > Nick
> > > > >> >
> > > > >> > On Wed, Jun 24, 2020 at 2:53 PM Sean Busbey
> > >
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
Agree on replacing VFT in next 2.4.0 or 3.0.0 release and restoring the required
method for now to unblock 2.3.0 RC1.
On 2020/06/26 01:11:31, Andrew Purtell wrote:
> Sounds fine to me.
>
> My earlier objection was to talk of an HBase 3 followed by an HBase 4. We
> don't need to do a full deprecation cycle across two major versions to
> remove an annotation that never promised public access. (By definition,
> tagged fields and members were VisibleForTesting (only). The 'only' was
> implied, but I think a reasonable assumption and common knowledge.)
>
> On Thu, Jun 25, 2020 at 3:48 PM Sean Busbey wrote:
>
> > Agree on restoring the member and then getting this done for 2.4.0.
> >
> >
> > On Thu, Jun 25, 2020, 15:02 Nick Dimiduk wrote:
> >
> > > And now by module,
> > >
> > > $ find . -iname '*.java' -exec grep -n '@VisibleForTesting' {} \+ | cut
> > -d/
> > > -f2 | sort | uniq -c
> > >6 hbase-backup
> > > 87 hbase-client
> > > 40 hbase-common
> > >1 hbase-endpoint
> > >7 hbase-hadoop-compat
> > >3 hbase-http
> > > 18 hbase-mapreduce
> > >1 hbase-metrics-api
> > > 24 hbase-procedure
> > > 10 hbase-replication
> > > 456 hbase-server
> > >2 hbase-thrift
> > >1 hbase-zookeeper
> > >
> > > I prefer we not make this change a prerequisite to 2.3. I would rather we
> > > restore the one method modified by HBASE-24221 and do the work for
> > > VisibleForTesting for 2.4.0.
> > >
> > > On Thu, Jun 25, 2020 at 12:57 PM Nick Dimiduk
> > wrote:
> > >
> > > > On Thu, Jun 25, 2020 at 12:36 PM Andrew Purtell
> > > > wrote:
> > > >
> > > >> I think we are in agreement except for a need to have a deprecation
> > > cycle.
> > > >> Just remove VisibleForTesting and replace with whatever alternative
> > you
> > > >> like. Certainly in the next minors. No strong opinion either way about
> > > >> patch releases, leave as is?
> > > >>
> > > >
> > > > Thanks Andrew and Bharath, I now better understand your positions.
> > > >
> > > > The annotation is fairly common in our codebase, from branch-2.3,
> > > >
> > > > $ find . -iname '*.java' -exec grep -n '@VisibleForTesting' {} \+ | wc
> > -l
> > > > 668
> > > >
> > > > I don't have an easy way to cross-reference this with our AI
> > annotations,
> > > > but my concern is that any change we make here without a deprecation
> > > cycle
> > > > will be disruptive to users.
> > > >
> > > > On Thu, Jun 25, 2020 at 11:30 AM Nick Dimiduk
> > > wrote:
> > > >>
> > > >> > On Wed, Jun 24, 2020 at 3:19 PM Andrew Purtell > >
> > > >> > wrote:
> > > >> >
> > > >> > > It is possible some users may not understand what Guava's
> > > >> > VisibleForTesting
> > > >> > > implies, but those users are much more likely to be Java
> > developers
> > > or
> > > >> > Java
> > > >> > > developer adjacent, and familiar with what this fad entailed. Such
> > > >> > tagging
> > > >> > > was/is done specifically to indicate the exposed field or method
> > was
> > > >> only
> > > >> > > made to allow test access to internals, as something less than
> > > public.
> > > >> > >
> > > >> > > For us to treat such annotated fields and methods as public after
> > > all
> > > >> is
> > > >> > > unnecessary, possibly surprising, and not semantically sound
> > (IMHO).
> > > >> > >
> > > >> >
> > > >> > I don't want to preserve use of VisibleForTesting as an indicator of
> > > >> public
> > > >> > API. I want to ensure that we're clear to our downstream users
> > > >> > that its presence is not a factor in determining public API. For
> > > >> example, I
> > > >> > don't want to update our book to give any meaning to this
> > annotation,
> > > >> and I
> > > >> > don't want to update our javadoc filters to take it into account
> > when
> > > >> > generating the various versions of javadoc that we publish. I want
> > to
> > > >> purge
> > > >> > it from the discussion by annotating the methods it decorates with
> > the
> > > >> > symbols we do use to define our public API. The steps I propose
> > above
> > > >> are
> > > >> > my suggestion of how we work toward that goal.
> > > >> >
> > > >> > Does anyone have a counter-proposal to the steps I've outlined
> > above?
> > > A
> > > >> > resolution to this discussion is now the final blocker on 2.3.0rc1.
> > > >> >
> > > >> > Thanks,
> > > >> > Nick
> > > >> >
> > > >> > On Wed, Jun 24, 2020 at 2:53 PM Sean Busbey
> > > wrote:
> > > >> > >
> > > >> > > > Andrew are you specifically opposed to using a deprecation cycle
> > > to
> > > >> > > > formally label as private anything that currently has a
> > > >> > VisibleForTesting
> > > >> > > > annotation?
> > > >> > > >
> > > >> > > > On Wed, Jun 24, 2020, 16:07 Andrew Purtell > >
> > > >> > wrote:
> > > >> > > >
> > > >> > > > > I am -1 on treating VisibleForTesting as public API.
> > > Semantically
> > > >> it
> > > >> > > > makes
> > > >> > > > > no sense.
> > > >> > > > >
> > > >> > > > > On Wed, Jun 24, 2020 at 12:22 PM Nick Dimiduk <
> > > >> ndimi...@apache.org>
> > > >>
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
Sounds fine to me.
My earlier objection was to talk of an HBase 3 followed by an HBase 4. We
don't need to do a full deprecation cycle across two major versions to
remove an annotation that never promised public access. (By definition,
tagged fields and members were VisibleForTesting (only). The 'only' was
implied, but I think a reasonable assumption and common knowledge.)
On Thu, Jun 25, 2020 at 3:48 PM Sean Busbey wrote:
> Agree on restoring the member and then getting this done for 2.4.0.
>
>
> On Thu, Jun 25, 2020, 15:02 Nick Dimiduk wrote:
>
> > And now by module,
> >
> > $ find . -iname '*.java' -exec grep -n '@VisibleForTesting' {} \+ | cut
> -d/
> > -f2 | sort | uniq -c
> >6 hbase-backup
> > 87 hbase-client
> > 40 hbase-common
> >1 hbase-endpoint
> >7 hbase-hadoop-compat
> >3 hbase-http
> > 18 hbase-mapreduce
> >1 hbase-metrics-api
> > 24 hbase-procedure
> > 10 hbase-replication
> > 456 hbase-server
> >2 hbase-thrift
> >1 hbase-zookeeper
> >
> > I prefer we not make this change a prerequisite to 2.3. I would rather we
> > restore the one method modified by HBASE-24221 and do the work for
> > VisibleForTesting for 2.4.0.
> >
> > On Thu, Jun 25, 2020 at 12:57 PM Nick Dimiduk
> wrote:
> >
> > > On Thu, Jun 25, 2020 at 12:36 PM Andrew Purtell
> > > wrote:
> > >
> > >> I think we are in agreement except for a need to have a deprecation
> > cycle.
> > >> Just remove VisibleForTesting and replace with whatever alternative
> you
> > >> like. Certainly in the next minors. No strong opinion either way about
> > >> patch releases, leave as is?
> > >>
> > >
> > > Thanks Andrew and Bharath, I now better understand your positions.
> > >
> > > The annotation is fairly common in our codebase, from branch-2.3,
> > >
> > > $ find . -iname '*.java' -exec grep -n '@VisibleForTesting' {} \+ | wc
> -l
> > > 668
> > >
> > > I don't have an easy way to cross-reference this with our AI
> annotations,
> > > but my concern is that any change we make here without a deprecation
> > cycle
> > > will be disruptive to users.
> > >
> > > On Thu, Jun 25, 2020 at 11:30 AM Nick Dimiduk
> > wrote:
> > >>
> > >> > On Wed, Jun 24, 2020 at 3:19 PM Andrew Purtell >
> > >> > wrote:
> > >> >
> > >> > > It is possible some users may not understand what Guava's
> > >> > VisibleForTesting
> > >> > > implies, but those users are much more likely to be Java
> developers
> > or
> > >> > Java
> > >> > > developer adjacent, and familiar with what this fad entailed. Such
> > >> > tagging
> > >> > > was/is done specifically to indicate the exposed field or method
> was
> > >> only
> > >> > > made to allow test access to internals, as something less than
> > public.
> > >> > >
> > >> > > For us to treat such annotated fields and methods as public after
> > all
> > >> is
> > >> > > unnecessary, possibly surprising, and not semantically sound
> (IMHO).
> > >> > >
> > >> >
> > >> > I don't want to preserve use of VisibleForTesting as an indicator of
> > >> public
> > >> > API. I want to ensure that we're clear to our downstream users
> > >> > that its presence is not a factor in determining public API. For
> > >> example, I
> > >> > don't want to update our book to give any meaning to this
> annotation,
> > >> and I
> > >> > don't want to update our javadoc filters to take it into account
> when
> > >> > generating the various versions of javadoc that we publish. I want
> to
> > >> purge
> > >> > it from the discussion by annotating the methods it decorates with
> the
> > >> > symbols we do use to define our public API. The steps I propose
> above
> > >> are
> > >> > my suggestion of how we work toward that goal.
> > >> >
> > >> > Does anyone have a counter-proposal to the steps I've outlined
> above?
> > A
> > >> > resolution to this discussion is now the final blocker on 2.3.0rc1.
> > >> >
> > >> > Thanks,
> > >> > Nick
> > >> >
> > >> > On Wed, Jun 24, 2020 at 2:53 PM Sean Busbey
> > wrote:
> > >> > >
> > >> > > > Andrew are you specifically opposed to using a deprecation cycle
> > to
> > >> > > > formally label as private anything that currently has a
> > >> > VisibleForTesting
> > >> > > > annotation?
> > >> > > >
> > >> > > > On Wed, Jun 24, 2020, 16:07 Andrew Purtell >
> > >> > wrote:
> > >> > > >
> > >> > > > > I am -1 on treating VisibleForTesting as public API.
> > Semantically
> > >> it
> > >> > > > makes
> > >> > > > > no sense.
> > >> > > > >
> > >> > > > > On Wed, Jun 24, 2020 at 12:22 PM Nick Dimiduk <
> > >> ndimi...@apache.org>
> > >> > > > wrote:
> > >> > > > >
> > >> > > > > > I'd like to get this [DISCUSS] wrapped up so we can proceed
> > with
> > >> > > > release
> > >> > > > > > candidates.
> > >> > > > > >
> > >> > > > > > I don't see a clear consensus here. The conclusion I read is
> > >> that
> > >> > > > > > developers generally intended the VisibleForTesting
> annotation
> > >> to
> > >> > > > > indicate
> > >> > > > > > a method is not a part of our public API, but
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
Agree on restoring the member and then getting this done for 2.4.0.
On Thu, Jun 25, 2020, 15:02 Nick Dimiduk wrote:
> And now by module,
>
> $ find . -iname '*.java' -exec grep -n '@VisibleForTesting' {} \+ | cut -d/
> -f2 | sort | uniq -c
>6 hbase-backup
> 87 hbase-client
> 40 hbase-common
>1 hbase-endpoint
>7 hbase-hadoop-compat
>3 hbase-http
> 18 hbase-mapreduce
>1 hbase-metrics-api
> 24 hbase-procedure
> 10 hbase-replication
> 456 hbase-server
>2 hbase-thrift
>1 hbase-zookeeper
>
> I prefer we not make this change a prerequisite to 2.3. I would rather we
> restore the one method modified by HBASE-24221 and do the work for
> VisibleForTesting for 2.4.0.
>
> On Thu, Jun 25, 2020 at 12:57 PM Nick Dimiduk wrote:
>
> > On Thu, Jun 25, 2020 at 12:36 PM Andrew Purtell
> > wrote:
> >
> >> I think we are in agreement except for a need to have a deprecation
> cycle.
> >> Just remove VisibleForTesting and replace with whatever alternative you
> >> like. Certainly in the next minors. No strong opinion either way about
> >> patch releases, leave as is?
> >>
> >
> > Thanks Andrew and Bharath, I now better understand your positions.
> >
> > The annotation is fairly common in our codebase, from branch-2.3,
> >
> > $ find . -iname '*.java' -exec grep -n '@VisibleForTesting' {} \+ | wc -l
> > 668
> >
> > I don't have an easy way to cross-reference this with our AI annotations,
> > but my concern is that any change we make here without a deprecation
> cycle
> > will be disruptive to users.
> >
> > On Thu, Jun 25, 2020 at 11:30 AM Nick Dimiduk
> wrote:
> >>
> >> > On Wed, Jun 24, 2020 at 3:19 PM Andrew Purtell
> >> > wrote:
> >> >
> >> > > It is possible some users may not understand what Guava's
> >> > VisibleForTesting
> >> > > implies, but those users are much more likely to be Java developers
> or
> >> > Java
> >> > > developer adjacent, and familiar with what this fad entailed. Such
> >> > tagging
> >> > > was/is done specifically to indicate the exposed field or method was
> >> only
> >> > > made to allow test access to internals, as something less than
> public.
> >> > >
> >> > > For us to treat such annotated fields and methods as public after
> all
> >> is
> >> > > unnecessary, possibly surprising, and not semantically sound (IMHO).
> >> > >
> >> >
> >> > I don't want to preserve use of VisibleForTesting as an indicator of
> >> public
> >> > API. I want to ensure that we're clear to our downstream users
> >> > that its presence is not a factor in determining public API. For
> >> example, I
> >> > don't want to update our book to give any meaning to this annotation,
> >> and I
> >> > don't want to update our javadoc filters to take it into account when
> >> > generating the various versions of javadoc that we publish. I want to
> >> purge
> >> > it from the discussion by annotating the methods it decorates with the
> >> > symbols we do use to define our public API. The steps I propose above
> >> are
> >> > my suggestion of how we work toward that goal.
> >> >
> >> > Does anyone have a counter-proposal to the steps I've outlined above?
> A
> >> > resolution to this discussion is now the final blocker on 2.3.0rc1.
> >> >
> >> > Thanks,
> >> > Nick
> >> >
> >> > On Wed, Jun 24, 2020 at 2:53 PM Sean Busbey
> wrote:
> >> > >
> >> > > > Andrew are you specifically opposed to using a deprecation cycle
> to
> >> > > > formally label as private anything that currently has a
> >> > VisibleForTesting
> >> > > > annotation?
> >> > > >
> >> > > > On Wed, Jun 24, 2020, 16:07 Andrew Purtell
> >> > wrote:
> >> > > >
> >> > > > > I am -1 on treating VisibleForTesting as public API.
> Semantically
> >> it
> >> > > > makes
> >> > > > > no sense.
> >> > > > >
> >> > > > > On Wed, Jun 24, 2020 at 12:22 PM Nick Dimiduk <
> >> ndimi...@apache.org>
> >> > > > wrote:
> >> > > > >
> >> > > > > > I'd like to get this [DISCUSS] wrapped up so we can proceed
> with
> >> > > > release
> >> > > > > > candidates.
> >> > > > > >
> >> > > > > > I don't see a clear consensus here. The conclusion I read is
> >> that
> >> > > > > > developers generally intended the VisibleForTesting annotation
> >> to
> >> > > > > indicate
> >> > > > > > a method is not a part of our public API, but because we don't
> >> > > > explicitly
> >> > > > > > say this in our guide, we can't really stand on that for the
> >> > > community.
> >> > > > > >
> >> > > > > > I propose we take the following, conservative steps going
> >> forward:
> >> > > > > >
> >> > > > > > 1. restore any VisibleForTesting method signatures for 2.3.0,
> >> treat
> >> > > > this
> >> > > > > as
> >> > > > > > public API going forward.
> >> > > > > > 2. annotate any existing methods carrying the
> VisibleForTesting
> >> > > > > annotation
> >> > > > > > as Deprecated in 2.3.x+, for removal in 4.0.0
> >> > > > > > 3. purge the VisibleForTesting annotation from our codebase
> for
> >> > > 4.0.0,
> >> > > > > > involving:
> >> > > > > > 3a. replace
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
And now by module,
$ find . -iname '*.java' -exec grep -n '@VisibleForTesting' {} \+ | cut -d/
-f2 | sort | uniq -c
6 hbase-backup
87 hbase-client
40 hbase-common
1 hbase-endpoint
7 hbase-hadoop-compat
3 hbase-http
18 hbase-mapreduce
1 hbase-metrics-api
24 hbase-procedure
10 hbase-replication
456 hbase-server
2 hbase-thrift
1 hbase-zookeeper
I prefer we not make this change a prerequisite to 2.3. I would rather we
restore the one method modified by HBASE-24221 and do the work for
VisibleForTesting for 2.4.0.
On Thu, Jun 25, 2020 at 12:57 PM Nick Dimiduk wrote:
> On Thu, Jun 25, 2020 at 12:36 PM Andrew Purtell
> wrote:
>
>> I think we are in agreement except for a need to have a deprecation cycle.
>> Just remove VisibleForTesting and replace with whatever alternative you
>> like. Certainly in the next minors. No strong opinion either way about
>> patch releases, leave as is?
>>
>
> Thanks Andrew and Bharath, I now better understand your positions.
>
> The annotation is fairly common in our codebase, from branch-2.3,
>
> $ find . -iname '*.java' -exec grep -n '@VisibleForTesting' {} \+ | wc -l
> 668
>
> I don't have an easy way to cross-reference this with our AI annotations,
> but my concern is that any change we make here without a deprecation cycle
> will be disruptive to users.
>
> On Thu, Jun 25, 2020 at 11:30 AM Nick Dimiduk wrote:
>>
>> > On Wed, Jun 24, 2020 at 3:19 PM Andrew Purtell
>> > wrote:
>> >
>> > > It is possible some users may not understand what Guava's
>> > VisibleForTesting
>> > > implies, but those users are much more likely to be Java developers or
>> > Java
>> > > developer adjacent, and familiar with what this fad entailed. Such
>> > tagging
>> > > was/is done specifically to indicate the exposed field or method was
>> only
>> > > made to allow test access to internals, as something less than public.
>> > >
>> > > For us to treat such annotated fields and methods as public after all
>> is
>> > > unnecessary, possibly surprising, and not semantically sound (IMHO).
>> > >
>> >
>> > I don't want to preserve use of VisibleForTesting as an indicator of
>> public
>> > API. I want to ensure that we're clear to our downstream users
>> > that its presence is not a factor in determining public API. For
>> example, I
>> > don't want to update our book to give any meaning to this annotation,
>> and I
>> > don't want to update our javadoc filters to take it into account when
>> > generating the various versions of javadoc that we publish. I want to
>> purge
>> > it from the discussion by annotating the methods it decorates with the
>> > symbols we do use to define our public API. The steps I propose above
>> are
>> > my suggestion of how we work toward that goal.
>> >
>> > Does anyone have a counter-proposal to the steps I've outlined above? A
>> > resolution to this discussion is now the final blocker on 2.3.0rc1.
>> >
>> > Thanks,
>> > Nick
>> >
>> > On Wed, Jun 24, 2020 at 2:53 PM Sean Busbey wrote:
>> > >
>> > > > Andrew are you specifically opposed to using a deprecation cycle to
>> > > > formally label as private anything that currently has a
>> > VisibleForTesting
>> > > > annotation?
>> > > >
>> > > > On Wed, Jun 24, 2020, 16:07 Andrew Purtell
>> > wrote:
>> > > >
>> > > > > I am -1 on treating VisibleForTesting as public API. Semantically
>> it
>> > > > makes
>> > > > > no sense.
>> > > > >
>> > > > > On Wed, Jun 24, 2020 at 12:22 PM Nick Dimiduk <
>> ndimi...@apache.org>
>> > > > wrote:
>> > > > >
>> > > > > > I'd like to get this [DISCUSS] wrapped up so we can proceed with
>> > > > release
>> > > > > > candidates.
>> > > > > >
>> > > > > > I don't see a clear consensus here. The conclusion I read is
>> that
>> > > > > > developers generally intended the VisibleForTesting annotation
>> to
>> > > > > indicate
>> > > > > > a method is not a part of our public API, but because we don't
>> > > > explicitly
>> > > > > > say this in our guide, we can't really stand on that for the
>> > > community.
>> > > > > >
>> > > > > > I propose we take the following, conservative steps going
>> forward:
>> > > > > >
>> > > > > > 1. restore any VisibleForTesting method signatures for 2.3.0,
>> treat
>> > > > this
>> > > > > as
>> > > > > > public API going forward.
>> > > > > > 2. annotate any existing methods carrying the VisibleForTesting
>> > > > > annotation
>> > > > > > as Deprecated in 2.3.x+, for removal in 4.0.0
>> > > > > > 3. purge the VisibleForTesting annotation from our codebase for
>> > > 4.0.0,
>> > > > > > involving:
>> > > > > > 3a. replace VisibleForTesting with IA.Private anywhere method
>> > > > visibility
>> > > > > > cannot be limited
>> > > > > > 3b. perhaps add a new Yetus check that would ban new use of
>> > > > > > VisibleForTesting
>> > > > > >
>> > > > > > Did I miss anything?
>> > > > > >
>> > > > > > Thanks,
>> > > > > > Nick
>> > > > > >
>> > > > > > On Wed, Jun 24, 2020 at 12:22 AM Viraj Jasani <
>> vjas...@apache.org>
>> > >
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
On Thu, Jun 25, 2020 at 12:36 PM Andrew Purtell wrote:
> I think we are in agreement except for a need to have a deprecation cycle.
> Just remove VisibleForTesting and replace with whatever alternative you
> like. Certainly in the next minors. No strong opinion either way about
> patch releases, leave as is?
>
Thanks Andrew and Bharath, I now better understand your positions.
The annotation is fairly common in our codebase, from branch-2.3,
$ find . -iname '*.java' -exec grep -n '@VisibleForTesting' {} \+ | wc -l
668
I don't have an easy way to cross-reference this with our AI annotations,
but my concern is that any change we make here without a deprecation cycle
will be disruptive to users.
On Thu, Jun 25, 2020 at 11:30 AM Nick Dimiduk wrote:
>
> > On Wed, Jun 24, 2020 at 3:19 PM Andrew Purtell
> > wrote:
> >
> > > It is possible some users may not understand what Guava's
> > VisibleForTesting
> > > implies, but those users are much more likely to be Java developers or
> > Java
> > > developer adjacent, and familiar with what this fad entailed. Such
> > tagging
> > > was/is done specifically to indicate the exposed field or method was
> only
> > > made to allow test access to internals, as something less than public.
> > >
> > > For us to treat such annotated fields and methods as public after all
> is
> > > unnecessary, possibly surprising, and not semantically sound (IMHO).
> > >
> >
> > I don't want to preserve use of VisibleForTesting as an indicator of
> public
> > API. I want to ensure that we're clear to our downstream users
> > that its presence is not a factor in determining public API. For
> example, I
> > don't want to update our book to give any meaning to this annotation,
> and I
> > don't want to update our javadoc filters to take it into account when
> > generating the various versions of javadoc that we publish. I want to
> purge
> > it from the discussion by annotating the methods it decorates with the
> > symbols we do use to define our public API. The steps I propose above are
> > my suggestion of how we work toward that goal.
> >
> > Does anyone have a counter-proposal to the steps I've outlined above? A
> > resolution to this discussion is now the final blocker on 2.3.0rc1.
> >
> > Thanks,
> > Nick
> >
> > On Wed, Jun 24, 2020 at 2:53 PM Sean Busbey wrote:
> > >
> > > > Andrew are you specifically opposed to using a deprecation cycle to
> > > > formally label as private anything that currently has a
> > VisibleForTesting
> > > > annotation?
> > > >
> > > > On Wed, Jun 24, 2020, 16:07 Andrew Purtell
> > wrote:
> > > >
> > > > > I am -1 on treating VisibleForTesting as public API. Semantically
> it
> > > > makes
> > > > > no sense.
> > > > >
> > > > > On Wed, Jun 24, 2020 at 12:22 PM Nick Dimiduk >
> > > > wrote:
> > > > >
> > > > > > I'd like to get this [DISCUSS] wrapped up so we can proceed with
> > > > release
> > > > > > candidates.
> > > > > >
> > > > > > I don't see a clear consensus here. The conclusion I read is that
> > > > > > developers generally intended the VisibleForTesting annotation to
> > > > > indicate
> > > > > > a method is not a part of our public API, but because we don't
> > > > explicitly
> > > > > > say this in our guide, we can't really stand on that for the
> > > community.
> > > > > >
> > > > > > I propose we take the following, conservative steps going
> forward:
> > > > > >
> > > > > > 1. restore any VisibleForTesting method signatures for 2.3.0,
> treat
> > > > this
> > > > > as
> > > > > > public API going forward.
> > > > > > 2. annotate any existing methods carrying the VisibleForTesting
> > > > > annotation
> > > > > > as Deprecated in 2.3.x+, for removal in 4.0.0
> > > > > > 3. purge the VisibleForTesting annotation from our codebase for
> > > 4.0.0,
> > > > > > involving:
> > > > > > 3a. replace VisibleForTesting with IA.Private anywhere method
> > > > visibility
> > > > > > cannot be limited
> > > > > > 3b. perhaps add a new Yetus check that would ban new use of
> > > > > > VisibleForTesting
> > > > > >
> > > > > > Did I miss anything?
> > > > > >
> > > > > > Thanks,
> > > > > > Nick
> > > > > >
> > > > > > On Wed, Jun 24, 2020 at 12:22 AM Viraj Jasani <
> vjas...@apache.org>
> > > > > wrote:
> > > > > >
> > > > > > > +1 to "be clear in javadoc" and to the fact that guava
> dependency
> > > > just
> > > > > to
> > > > > > > express intention which can be done through javadoc is not
> > > > > > > required unless the library is capable of breaking compilation
> of
> > > > > > > downstream
> > > > > > > projects if they use VFT annotated classes/methods saying you
> > can't
> > > > use
> > > > > > > this
> > > > > > > (what if we have such fancy thing? :) ).
> > > > > > >
> > > > > > >
> > > > > > > On 2020/06/23 20:01:40, Sean Busbey wrote:
> > > > > > > > +1 to "do it in javadoc" unless there's some magic for IDEs
> > > brought
> > > > > > about
> > > > > > > > via the VFT annotation that I'm missing.
> > > > > > > >
> > > > > > >
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
I think we are in agreement except for a need to have a deprecation cycle. Just remove VisibleForTesting and replace with whatever alternative you like. Certainly in the next minors. No strong opinion either way about patch releases, leave as is? On Thu, Jun 25, 2020 at 11:30 AM Nick Dimiduk wrote: > On Wed, Jun 24, 2020 at 3:19 PM Andrew Purtell > wrote: > > > It is possible some users may not understand what Guava's > VisibleForTesting > > implies, but those users are much more likely to be Java developers or > Java > > developer adjacent, and familiar with what this fad entailed. Such > tagging > > was/is done specifically to indicate the exposed field or method was only > > made to allow test access to internals, as something less than public. > > > > For us to treat such annotated fields and methods as public after all is > > unnecessary, possibly surprising, and not semantically sound (IMHO). > > > > I don't want to preserve use of VisibleForTesting as an indicator of public > API. I want to ensure that we're clear to our downstream users > that its presence is not a factor in determining public API. For example, I > don't want to update our book to give any meaning to this annotation, and I > don't want to update our javadoc filters to take it into account when > generating the various versions of javadoc that we publish. I want to purge > it from the discussion by annotating the methods it decorates with the > symbols we do use to define our public API. The steps I propose above are > my suggestion of how we work toward that goal. > > Does anyone have a counter-proposal to the steps I've outlined above? A > resolution to this discussion is now the final blocker on 2.3.0rc1. > > Thanks, > Nick > > On Wed, Jun 24, 2020 at 2:53 PM Sean Busbey wrote: > > > > > Andrew are you specifically opposed to using a deprecation cycle to > > > formally label as private anything that currently has a > VisibleForTesting > > > annotation? > > > > > > On Wed, Jun 24, 2020, 16:07 Andrew Purtell > wrote: > > > > > > > I am -1 on treating VisibleForTesting as public API. Semantically it > > > makes > > > > no sense. > > > > > > > > On Wed, Jun 24, 2020 at 12:22 PM Nick Dimiduk > > > wrote: > > > > > > > > > I'd like to get this [DISCUSS] wrapped up so we can proceed with > > > release > > > > > candidates. > > > > > > > > > > I don't see a clear consensus here. The conclusion I read is that > > > > > developers generally intended the VisibleForTesting annotation to > > > > indicate > > > > > a method is not a part of our public API, but because we don't > > > explicitly > > > > > say this in our guide, we can't really stand on that for the > > community. > > > > > > > > > > I propose we take the following, conservative steps going forward: > > > > > > > > > > 1. restore any VisibleForTesting method signatures for 2.3.0, treat > > > this > > > > as > > > > > public API going forward. > > > > > 2. annotate any existing methods carrying the VisibleForTesting > > > > annotation > > > > > as Deprecated in 2.3.x+, for removal in 4.0.0 > > > > > 3. purge the VisibleForTesting annotation from our codebase for > > 4.0.0, > > > > > involving: > > > > > 3a. replace VisibleForTesting with IA.Private anywhere method > > > visibility > > > > > cannot be limited > > > > > 3b. perhaps add a new Yetus check that would ban new use of > > > > > VisibleForTesting > > > > > > > > > > Did I miss anything? > > > > > > > > > > Thanks, > > > > > Nick > > > > > > > > > > On Wed, Jun 24, 2020 at 12:22 AM Viraj Jasani > > > > wrote: > > > > > > > > > > > +1 to "be clear in javadoc" and to the fact that guava dependency > > > just > > > > to > > > > > > express intention which can be done through javadoc is not > > > > > > required unless the library is capable of breaking compilation of > > > > > > downstream > > > > > > projects if they use VFT annotated classes/methods saying you > can't > > > use > > > > > > this > > > > > > (what if we have such fancy thing? :) ). > > > > > > > > > > > > > > > > > > On 2020/06/23 20:01:40, Sean Busbey wrote: > > > > > > > +1 to "do it in javadoc" unless there's some magic for IDEs > > brought > > > > > about > > > > > > > via the VFT annotation that I'm missing. > > > > > > > > > > > > > > On Tue, Jun 23, 2020, 13:04 Andrew Purtell < > apurt...@apache.org> > > > > > wrote: > > > > > > > > > > > > > > > I don't find the VisibleForTesting annotation provides a lot > of > > > > > value. > > > > > > It > > > > > > > > became fashionable to use this annotation when a single line > of > > > > > Javadoc > > > > > > > > would serve the same purpose and not make yet another > > dependency > > > on > > > > > > Guava. > > > > > > > > My advice is to remove them all and replace with Javadoc. > > > > > > > > > > > > > > > > Even if in an IA.Public or LimitedPrivate we can decorate > > > > individual > > > > > > > > field/methods that are public but not intended to be part of > > the > > > > > public > > > > > >
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
Also to be clear, I don't have any objections with the steps outlined. so +1 on the depreciation schedule. On Thu, Jun 25, 2020 at 11:43 AM Bharath Vissapragada wrote: > +1 on standardization using I.A rather than guava annotations. > > On Thu, Jun 25, 2020 at 11:30 AM Nick Dimiduk wrote: > >> On Wed, Jun 24, 2020 at 3:19 PM Andrew Purtell >> wrote: >> >> > It is possible some users may not understand what Guava's >> VisibleForTesting >> > implies, but those users are much more likely to be Java developers or >> Java >> > developer adjacent, and familiar with what this fad entailed. Such >> tagging >> > was/is done specifically to indicate the exposed field or method was >> only >> > made to allow test access to internals, as something less than public. >> > >> > For us to treat such annotated fields and methods as public after all is >> > unnecessary, possibly surprising, and not semantically sound (IMHO). >> > >> >> I don't want to preserve use of VisibleForTesting as an indicator of >> public >> API. I want to ensure that we're clear to our downstream users >> that its presence is not a factor in determining public API. For example, >> I >> don't want to update our book to give any meaning to this annotation, and >> I >> don't want to update our javadoc filters to take it into account when >> generating the various versions of javadoc that we publish. I want to >> purge >> it from the discussion by annotating the methods it decorates with the >> symbols we do use to define our public API. The steps I propose above are >> my suggestion of how we work toward that goal. >> >> Does anyone have a counter-proposal to the steps I've outlined above? A >> resolution to this discussion is now the final blocker on 2.3.0rc1. >> >> Thanks, >> Nick >> >> On Wed, Jun 24, 2020 at 2:53 PM Sean Busbey wrote: >> > >> > > Andrew are you specifically opposed to using a deprecation cycle to >> > > formally label as private anything that currently has a >> VisibleForTesting >> > > annotation? >> > > >> > > On Wed, Jun 24, 2020, 16:07 Andrew Purtell >> wrote: >> > > >> > > > I am -1 on treating VisibleForTesting as public API. Semantically it >> > > makes >> > > > no sense. >> > > > >> > > > On Wed, Jun 24, 2020 at 12:22 PM Nick Dimiduk >> > > wrote: >> > > > >> > > > > I'd like to get this [DISCUSS] wrapped up so we can proceed with >> > > release >> > > > > candidates. >> > > > > >> > > > > I don't see a clear consensus here. The conclusion I read is that >> > > > > developers generally intended the VisibleForTesting annotation to >> > > > indicate >> > > > > a method is not a part of our public API, but because we don't >> > > explicitly >> > > > > say this in our guide, we can't really stand on that for the >> > community. >> > > > > >> > > > > I propose we take the following, conservative steps going forward: >> > > > > >> > > > > 1. restore any VisibleForTesting method signatures for 2.3.0, >> treat >> > > this >> > > > as >> > > > > public API going forward. >> > > > > 2. annotate any existing methods carrying the VisibleForTesting >> > > > annotation >> > > > > as Deprecated in 2.3.x+, for removal in 4.0.0 >> > > > > 3. purge the VisibleForTesting annotation from our codebase for >> > 4.0.0, >> > > > > involving: >> > > > > 3a. replace VisibleForTesting with IA.Private anywhere method >> > > visibility >> > > > > cannot be limited >> > > > > 3b. perhaps add a new Yetus check that would ban new use of >> > > > > VisibleForTesting >> > > > > >> > > > > Did I miss anything? >> > > > > >> > > > > Thanks, >> > > > > Nick >> > > > > >> > > > > On Wed, Jun 24, 2020 at 12:22 AM Viraj Jasani > > >> > > > wrote: >> > > > > >> > > > > > +1 to "be clear in javadoc" and to the fact that guava >> dependency >> > > just >> > > > to >> > > > > > express intention which can be done through javadoc is not >> > > > > > required unless the library is capable of breaking compilation >> of >> > > > > > downstream >> > > > > > projects if they use VFT annotated classes/methods saying you >> can't >> > > use >> > > > > > this >> > > > > > (what if we have such fancy thing? :) ). >> > > > > > >> > > > > > >> > > > > > On 2020/06/23 20:01:40, Sean Busbey wrote: >> > > > > > > +1 to "do it in javadoc" unless there's some magic for IDEs >> > brought >> > > > > about >> > > > > > > via the VFT annotation that I'm missing. >> > > > > > > >> > > > > > > On Tue, Jun 23, 2020, 13:04 Andrew Purtell < >> apurt...@apache.org> >> > > > > wrote: >> > > > > > > >> > > > > > > > I don't find the VisibleForTesting annotation provides a >> lot of >> > > > > value. >> > > > > > It >> > > > > > > > became fashionable to use this annotation when a single >> line of >> > > > > Javadoc >> > > > > > > > would serve the same purpose and not make yet another >> > dependency >> > > on >> > > > > > Guava. >> > > > > > > > My advice is to remove them all and replace with Javadoc. >> > > > > > > > >> > > > > > > > Even if in an IA.Public or LimitedPrivate we can
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
+1 on standardization using I.A rather than guava annotations. On Thu, Jun 25, 2020 at 11:30 AM Nick Dimiduk wrote: > On Wed, Jun 24, 2020 at 3:19 PM Andrew Purtell > wrote: > > > It is possible some users may not understand what Guava's > VisibleForTesting > > implies, but those users are much more likely to be Java developers or > Java > > developer adjacent, and familiar with what this fad entailed. Such > tagging > > was/is done specifically to indicate the exposed field or method was only > > made to allow test access to internals, as something less than public. > > > > For us to treat such annotated fields and methods as public after all is > > unnecessary, possibly surprising, and not semantically sound (IMHO). > > > > I don't want to preserve use of VisibleForTesting as an indicator of public > API. I want to ensure that we're clear to our downstream users > that its presence is not a factor in determining public API. For example, I > don't want to update our book to give any meaning to this annotation, and I > don't want to update our javadoc filters to take it into account when > generating the various versions of javadoc that we publish. I want to purge > it from the discussion by annotating the methods it decorates with the > symbols we do use to define our public API. The steps I propose above are > my suggestion of how we work toward that goal. > > Does anyone have a counter-proposal to the steps I've outlined above? A > resolution to this discussion is now the final blocker on 2.3.0rc1. > > Thanks, > Nick > > On Wed, Jun 24, 2020 at 2:53 PM Sean Busbey wrote: > > > > > Andrew are you specifically opposed to using a deprecation cycle to > > > formally label as private anything that currently has a > VisibleForTesting > > > annotation? > > > > > > On Wed, Jun 24, 2020, 16:07 Andrew Purtell > wrote: > > > > > > > I am -1 on treating VisibleForTesting as public API. Semantically it > > > makes > > > > no sense. > > > > > > > > On Wed, Jun 24, 2020 at 12:22 PM Nick Dimiduk > > > wrote: > > > > > > > > > I'd like to get this [DISCUSS] wrapped up so we can proceed with > > > release > > > > > candidates. > > > > > > > > > > I don't see a clear consensus here. The conclusion I read is that > > > > > developers generally intended the VisibleForTesting annotation to > > > > indicate > > > > > a method is not a part of our public API, but because we don't > > > explicitly > > > > > say this in our guide, we can't really stand on that for the > > community. > > > > > > > > > > I propose we take the following, conservative steps going forward: > > > > > > > > > > 1. restore any VisibleForTesting method signatures for 2.3.0, treat > > > this > > > > as > > > > > public API going forward. > > > > > 2. annotate any existing methods carrying the VisibleForTesting > > > > annotation > > > > > as Deprecated in 2.3.x+, for removal in 4.0.0 > > > > > 3. purge the VisibleForTesting annotation from our codebase for > > 4.0.0, > > > > > involving: > > > > > 3a. replace VisibleForTesting with IA.Private anywhere method > > > visibility > > > > > cannot be limited > > > > > 3b. perhaps add a new Yetus check that would ban new use of > > > > > VisibleForTesting > > > > > > > > > > Did I miss anything? > > > > > > > > > > Thanks, > > > > > Nick > > > > > > > > > > On Wed, Jun 24, 2020 at 12:22 AM Viraj Jasani > > > > wrote: > > > > > > > > > > > +1 to "be clear in javadoc" and to the fact that guava dependency > > > just > > > > to > > > > > > express intention which can be done through javadoc is not > > > > > > required unless the library is capable of breaking compilation of > > > > > > downstream > > > > > > projects if they use VFT annotated classes/methods saying you > can't > > > use > > > > > > this > > > > > > (what if we have such fancy thing? :) ). > > > > > > > > > > > > > > > > > > On 2020/06/23 20:01:40, Sean Busbey wrote: > > > > > > > +1 to "do it in javadoc" unless there's some magic for IDEs > > brought > > > > > about > > > > > > > via the VFT annotation that I'm missing. > > > > > > > > > > > > > > On Tue, Jun 23, 2020, 13:04 Andrew Purtell < > apurt...@apache.org> > > > > > wrote: > > > > > > > > > > > > > > > I don't find the VisibleForTesting annotation provides a lot > of > > > > > value. > > > > > > It > > > > > > > > became fashionable to use this annotation when a single line > of > > > > > Javadoc > > > > > > > > would serve the same purpose and not make yet another > > dependency > > > on > > > > > > Guava. > > > > > > > > My advice is to remove them all and replace with Javadoc. > > > > > > > > > > > > > > > > Even if in an IA.Public or LimitedPrivate we can decorate > > > > individual > > > > > > > > field/methods that are public but not intended to be part of > > the > > > > > public > > > > > > > > portion of the API with a field or method level IA.Private > > > > > decoration. > > > > > > It's > > > > > > > > maybe not nice to do, but that would directly and clearly > >
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
On Wed, Jun 24, 2020 at 3:19 PM Andrew Purtell wrote: > It is possible some users may not understand what Guava's VisibleForTesting > implies, but those users are much more likely to be Java developers or Java > developer adjacent, and familiar with what this fad entailed. Such tagging > was/is done specifically to indicate the exposed field or method was only > made to allow test access to internals, as something less than public. > > For us to treat such annotated fields and methods as public after all is > unnecessary, possibly surprising, and not semantically sound (IMHO). > I don't want to preserve use of VisibleForTesting as an indicator of public API. I want to ensure that we're clear to our downstream users that its presence is not a factor in determining public API. For example, I don't want to update our book to give any meaning to this annotation, and I don't want to update our javadoc filters to take it into account when generating the various versions of javadoc that we publish. I want to purge it from the discussion by annotating the methods it decorates with the symbols we do use to define our public API. The steps I propose above are my suggestion of how we work toward that goal. Does anyone have a counter-proposal to the steps I've outlined above? A resolution to this discussion is now the final blocker on 2.3.0rc1. Thanks, Nick On Wed, Jun 24, 2020 at 2:53 PM Sean Busbey wrote: > > > Andrew are you specifically opposed to using a deprecation cycle to > > formally label as private anything that currently has a VisibleForTesting > > annotation? > > > > On Wed, Jun 24, 2020, 16:07 Andrew Purtell wrote: > > > > > I am -1 on treating VisibleForTesting as public API. Semantically it > > makes > > > no sense. > > > > > > On Wed, Jun 24, 2020 at 12:22 PM Nick Dimiduk > > wrote: > > > > > > > I'd like to get this [DISCUSS] wrapped up so we can proceed with > > release > > > > candidates. > > > > > > > > I don't see a clear consensus here. The conclusion I read is that > > > > developers generally intended the VisibleForTesting annotation to > > > indicate > > > > a method is not a part of our public API, but because we don't > > explicitly > > > > say this in our guide, we can't really stand on that for the > community. > > > > > > > > I propose we take the following, conservative steps going forward: > > > > > > > > 1. restore any VisibleForTesting method signatures for 2.3.0, treat > > this > > > as > > > > public API going forward. > > > > 2. annotate any existing methods carrying the VisibleForTesting > > > annotation > > > > as Deprecated in 2.3.x+, for removal in 4.0.0 > > > > 3. purge the VisibleForTesting annotation from our codebase for > 4.0.0, > > > > involving: > > > > 3a. replace VisibleForTesting with IA.Private anywhere method > > visibility > > > > cannot be limited > > > > 3b. perhaps add a new Yetus check that would ban new use of > > > > VisibleForTesting > > > > > > > > Did I miss anything? > > > > > > > > Thanks, > > > > Nick > > > > > > > > On Wed, Jun 24, 2020 at 12:22 AM Viraj Jasani > > > wrote: > > > > > > > > > +1 to "be clear in javadoc" and to the fact that guava dependency > > just > > > to > > > > > express intention which can be done through javadoc is not > > > > > required unless the library is capable of breaking compilation of > > > > > downstream > > > > > projects if they use VFT annotated classes/methods saying you can't > > use > > > > > this > > > > > (what if we have such fancy thing? :) ). > > > > > > > > > > > > > > > On 2020/06/23 20:01:40, Sean Busbey wrote: > > > > > > +1 to "do it in javadoc" unless there's some magic for IDEs > brought > > > > about > > > > > > via the VFT annotation that I'm missing. > > > > > > > > > > > > On Tue, Jun 23, 2020, 13:04 Andrew Purtell > > > > wrote: > > > > > > > > > > > > > I don't find the VisibleForTesting annotation provides a lot of > > > > value. > > > > > It > > > > > > > became fashionable to use this annotation when a single line of > > > > Javadoc > > > > > > > would serve the same purpose and not make yet another > dependency > > on > > > > > Guava. > > > > > > > My advice is to remove them all and replace with Javadoc. > > > > > > > > > > > > > > Even if in an IA.Public or LimitedPrivate we can decorate > > > individual > > > > > > > field/methods that are public but not intended to be part of > the > > > > public > > > > > > > portion of the API with a field or method level IA.Private > > > > decoration. > > > > > It's > > > > > > > maybe not nice to do, but that would directly and clearly > express > > > > > intent. > > > > > > > > > > > > > > On Tue, Jun 23, 2020 at 10:15 AM Sean Busbey < > bus...@apache.org> > > > > > wrote: > > > > > > > > > > > > > > > I think the intent behind VisibleForTesting is clear: the > > person > > > > > using > > > > > > > that > > > > > > > > annotation does not intend for it to be used by > downstreamers. > > > > > > > > > > > > > > > > However, our stated
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
It is possible some users may not understand what Guava's VisibleForTesting implies, but those users are much more likely to be Java developers or Java developer adjacent, and familiar with what this fad entailed. Such tagging was/is done specifically to indicate the exposed field or method was only made to allow test access to internals, as something less than public. For us to treat such annotated fields and methods as public after all is unnecessary, possibly surprising, and not semantically sound (IMHO). On Wed, Jun 24, 2020 at 2:53 PM Sean Busbey wrote: > Andrew are you specifically opposed to using a deprecation cycle to > formally label as private anything that currently has a VisibleForTesting > annotation? > > On Wed, Jun 24, 2020, 16:07 Andrew Purtell wrote: > > > I am -1 on treating VisibleForTesting as public API. Semantically it > makes > > no sense. > > > > On Wed, Jun 24, 2020 at 12:22 PM Nick Dimiduk > wrote: > > > > > I'd like to get this [DISCUSS] wrapped up so we can proceed with > release > > > candidates. > > > > > > I don't see a clear consensus here. The conclusion I read is that > > > developers generally intended the VisibleForTesting annotation to > > indicate > > > a method is not a part of our public API, but because we don't > explicitly > > > say this in our guide, we can't really stand on that for the community. > > > > > > I propose we take the following, conservative steps going forward: > > > > > > 1. restore any VisibleForTesting method signatures for 2.3.0, treat > this > > as > > > public API going forward. > > > 2. annotate any existing methods carrying the VisibleForTesting > > annotation > > > as Deprecated in 2.3.x+, for removal in 4.0.0 > > > 3. purge the VisibleForTesting annotation from our codebase for 4.0.0, > > > involving: > > > 3a. replace VisibleForTesting with IA.Private anywhere method > visibility > > > cannot be limited > > > 3b. perhaps add a new Yetus check that would ban new use of > > > VisibleForTesting > > > > > > Did I miss anything? > > > > > > Thanks, > > > Nick > > > > > > On Wed, Jun 24, 2020 at 12:22 AM Viraj Jasani > > wrote: > > > > > > > +1 to "be clear in javadoc" and to the fact that guava dependency > just > > to > > > > express intention which can be done through javadoc is not > > > > required unless the library is capable of breaking compilation of > > > > downstream > > > > projects if they use VFT annotated classes/methods saying you can't > use > > > > this > > > > (what if we have such fancy thing? :) ). > > > > > > > > > > > > On 2020/06/23 20:01:40, Sean Busbey wrote: > > > > > +1 to "do it in javadoc" unless there's some magic for IDEs brought > > > about > > > > > via the VFT annotation that I'm missing. > > > > > > > > > > On Tue, Jun 23, 2020, 13:04 Andrew Purtell > > > wrote: > > > > > > > > > > > I don't find the VisibleForTesting annotation provides a lot of > > > value. > > > > It > > > > > > became fashionable to use this annotation when a single line of > > > Javadoc > > > > > > would serve the same purpose and not make yet another dependency > on > > > > Guava. > > > > > > My advice is to remove them all and replace with Javadoc. > > > > > > > > > > > > Even if in an IA.Public or LimitedPrivate we can decorate > > individual > > > > > > field/methods that are public but not intended to be part of the > > > public > > > > > > portion of the API with a field or method level IA.Private > > > decoration. > > > > It's > > > > > > maybe not nice to do, but that would directly and clearly express > > > > intent. > > > > > > > > > > > > On Tue, Jun 23, 2020 at 10:15 AM Sean Busbey > > > > wrote: > > > > > > > > > > > > > I think the intent behind VisibleForTesting is clear: the > person > > > > using > > > > > > that > > > > > > > annotation does not intend for it to be used by downstreamers. > > > > > > > > > > > > > > However, our stated API promises are in terms of the Interface > > > > Audience > > > > > > > annotations only. So I think a downsteamer who e.g. used > > automated > > > > > > tooling > > > > > > > to ensure they only used things marked IA.Public would be > correct > > > to > > > > be > > > > > > > upset with us if we incompatibly changed an IA.Public member > that > > > is > > > > > > > annotated VisibleForTesting. > > > > > > > > > > > > > > Given that VisibleForTesting is in guava and we go to pains to > > > about > > > > > > > exposing downstream to non-relocated guava I think it would be > a > > > bad > > > > idea > > > > > > > to use it when defining our public API. > > > > > > > > > > > > > > We should find places that use it, make sure they also carry an > > > > > > IA.Private > > > > > > > if needed, and make sure our docs for developers are clear > about > > > > which > > > > > > > annotations carry meaning for downstreamers (i.e. only > Interface > > > > Audience > > > > > > > and Interface Stability). > > > > > > > > > > > > > > On Tue, Jun 23, 2020, 11:29 Nick Dimiduk > > > > wrote:
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
Andrew are you specifically opposed to using a deprecation cycle to formally label as private anything that currently has a VisibleForTesting annotation? On Wed, Jun 24, 2020, 16:07 Andrew Purtell wrote: > I am -1 on treating VisibleForTesting as public API. Semantically it makes > no sense. > > On Wed, Jun 24, 2020 at 12:22 PM Nick Dimiduk wrote: > > > I'd like to get this [DISCUSS] wrapped up so we can proceed with release > > candidates. > > > > I don't see a clear consensus here. The conclusion I read is that > > developers generally intended the VisibleForTesting annotation to > indicate > > a method is not a part of our public API, but because we don't explicitly > > say this in our guide, we can't really stand on that for the community. > > > > I propose we take the following, conservative steps going forward: > > > > 1. restore any VisibleForTesting method signatures for 2.3.0, treat this > as > > public API going forward. > > 2. annotate any existing methods carrying the VisibleForTesting > annotation > > as Deprecated in 2.3.x+, for removal in 4.0.0 > > 3. purge the VisibleForTesting annotation from our codebase for 4.0.0, > > involving: > > 3a. replace VisibleForTesting with IA.Private anywhere method visibility > > cannot be limited > > 3b. perhaps add a new Yetus check that would ban new use of > > VisibleForTesting > > > > Did I miss anything? > > > > Thanks, > > Nick > > > > On Wed, Jun 24, 2020 at 12:22 AM Viraj Jasani > wrote: > > > > > +1 to "be clear in javadoc" and to the fact that guava dependency just > to > > > express intention which can be done through javadoc is not > > > required unless the library is capable of breaking compilation of > > > downstream > > > projects if they use VFT annotated classes/methods saying you can't use > > > this > > > (what if we have such fancy thing? :) ). > > > > > > > > > On 2020/06/23 20:01:40, Sean Busbey wrote: > > > > +1 to "do it in javadoc" unless there's some magic for IDEs brought > > about > > > > via the VFT annotation that I'm missing. > > > > > > > > On Tue, Jun 23, 2020, 13:04 Andrew Purtell > > wrote: > > > > > > > > > I don't find the VisibleForTesting annotation provides a lot of > > value. > > > It > > > > > became fashionable to use this annotation when a single line of > > Javadoc > > > > > would serve the same purpose and not make yet another dependency on > > > Guava. > > > > > My advice is to remove them all and replace with Javadoc. > > > > > > > > > > Even if in an IA.Public or LimitedPrivate we can decorate > individual > > > > > field/methods that are public but not intended to be part of the > > public > > > > > portion of the API with a field or method level IA.Private > > decoration. > > > It's > > > > > maybe not nice to do, but that would directly and clearly express > > > intent. > > > > > > > > > > On Tue, Jun 23, 2020 at 10:15 AM Sean Busbey > > > wrote: > > > > > > > > > > > I think the intent behind VisibleForTesting is clear: the person > > > using > > > > > that > > > > > > annotation does not intend for it to be used by downstreamers. > > > > > > > > > > > > However, our stated API promises are in terms of the Interface > > > Audience > > > > > > annotations only. So I think a downsteamer who e.g. used > automated > > > > > tooling > > > > > > to ensure they only used things marked IA.Public would be correct > > to > > > be > > > > > > upset with us if we incompatibly changed an IA.Public member that > > is > > > > > > annotated VisibleForTesting. > > > > > > > > > > > > Given that VisibleForTesting is in guava and we go to pains to > > about > > > > > > exposing downstream to non-relocated guava I think it would be a > > bad > > > idea > > > > > > to use it when defining our public API. > > > > > > > > > > > > We should find places that use it, make sure they also carry an > > > > > IA.Private > > > > > > if needed, and make sure our docs for developers are clear about > > > which > > > > > > annotations carry meaning for downstreamers (i.e. only Interface > > > Audience > > > > > > and Interface Stability). > > > > > > > > > > > > On Tue, Jun 23, 2020, 11:29 Nick Dimiduk > > > wrote: > > > > > > > > > > > > > My hope is that we can clarify our policy and update the book > > > > > > accordingly. > > > > > > > > > > > > > > On Tue, Jun 23, 2020 at 9:01 AM Wellington Chevreuil < > > > > > > > wellington.chevre...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > For the current problem, what is the class? I think > changing > > a > > > > > method > > > > > > > > > signature for a protected method will only break the > > > compatibility > > > > > > when > > > > > > > > > users extend the class. > > > > > > > > > > > > > > > > > This specific case is > > *LoadIncrementalHFiles.tryAtomicRegionLoad, > > > > > > *mostly > > > > > > > > an end user tool, not likely to be extended. Bring back the > > > original > > > > > > > method > > > > > > > > would not be much of an issue, though, I
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
As I suggested before, replace them with Javadoc. On Wed, Jun 24, 2020 at 2:10 PM Nick Dimiduk wrote: > On Wed, Jun 24, 2020 at 14:07 Andrew Purtell wrote: > > > I am -1 on treating VisibleForTesting as public API. Semantically it > makes > > no sense. > > > By extension then you think it’s acceptable to define our public api in > terms of this annotation provided by Guava? I think that makes no sense. > > On Wed, Jun 24, 2020 at 12:22 PM Nick Dimiduk wrote: > > > > > I'd like to get this [DISCUSS] wrapped up so we can proceed with > release > > > candidates. > > > > > > I don't see a clear consensus here. The conclusion I read is that > > > developers generally intended the VisibleForTesting annotation to > > indicate > > > a method is not a part of our public API, but because we don't > explicitly > > > say this in our guide, we can't really stand on that for the community. > > > > > > I propose we take the following, conservative steps going forward: > > > > > > 1. restore any VisibleForTesting method signatures for 2.3.0, treat > this > > as > > > public API going forward. > > > 2. annotate any existing methods carrying the VisibleForTesting > > annotation > > > as Deprecated in 2.3.x+, for removal in 4.0.0 > > > 3. purge the VisibleForTesting annotation from our codebase for 4.0.0, > > > involving: > > > 3a. replace VisibleForTesting with IA.Private anywhere method > visibility > > > cannot be limited > > > 3b. perhaps add a new Yetus check that would ban new use of > > > VisibleForTesting > > > > > > Did I miss anything? > > > > > > Thanks, > > > Nick > > > > > > On Wed, Jun 24, 2020 at 12:22 AM Viraj Jasani > > wrote: > > > > > > > +1 to "be clear in javadoc" and to the fact that guava dependency > just > > to > > > > express intention which can be done through javadoc is not > > > > required unless the library is capable of breaking compilation of > > > > downstream > > > > projects if they use VFT annotated classes/methods saying you can't > use > > > > this > > > > (what if we have such fancy thing? :) ). > > > > > > > > > > > > On 2020/06/23 20:01:40, Sean Busbey wrote: > > > > > +1 to "do it in javadoc" unless there's some magic for IDEs brought > > > about > > > > > via the VFT annotation that I'm missing. > > > > > > > > > > On Tue, Jun 23, 2020, 13:04 Andrew Purtell > > > wrote: > > > > > > > > > > > I don't find the VisibleForTesting annotation provides a lot of > > > value. > > > > It > > > > > > became fashionable to use this annotation when a single line of > > > Javadoc > > > > > > would serve the same purpose and not make yet another dependency > on > > > > Guava. > > > > > > My advice is to remove them all and replace with Javadoc. > > > > > > > > > > > > Even if in an IA.Public or LimitedPrivate we can decorate > > individual > > > > > > field/methods that are public but not intended to be part of the > > > public > > > > > > portion of the API with a field or method level IA.Private > > > decoration. > > > > It's > > > > > > maybe not nice to do, but that would directly and clearly express > > > > intent. > > > > > > > > > > > > On Tue, Jun 23, 2020 at 10:15 AM Sean Busbey > > > > wrote: > > > > > > > > > > > > > I think the intent behind VisibleForTesting is clear: the > person > > > > using > > > > > > that > > > > > > > annotation does not intend for it to be used by downstreamers. > > > > > > > > > > > > > > However, our stated API promises are in terms of the Interface > > > > Audience > > > > > > > annotations only. So I think a downsteamer who e.g. used > > automated > > > > > > tooling > > > > > > > to ensure they only used things marked IA.Public would be > correct > > > to > > > > be > > > > > > > upset with us if we incompatibly changed an IA.Public member > that > > > is > > > > > > > annotated VisibleForTesting. > > > > > > > > > > > > > > Given that VisibleForTesting is in guava and we go to pains to > > > about > > > > > > > exposing downstream to non-relocated guava I think it would be > a > > > bad > > > > idea > > > > > > > to use it when defining our public API. > > > > > > > > > > > > > > We should find places that use it, make sure they also carry an > > > > > > IA.Private > > > > > > > if needed, and make sure our docs for developers are clear > about > > > > which > > > > > > > annotations carry meaning for downstreamers (i.e. only > Interface > > > > Audience > > > > > > > and Interface Stability). > > > > > > > > > > > > > > On Tue, Jun 23, 2020, 11:29 Nick Dimiduk > > > > wrote: > > > > > > > > > > > > > > > My hope is that we can clarify our policy and update the book > > > > > > > accordingly. > > > > > > > > > > > > > > > > On Tue, Jun 23, 2020 at 9:01 AM Wellington Chevreuil < > > > > > > > > wellington.chevre...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > For the current problem, what is the class? I think > > changing > > > a > > > > > > method > > > > > > > > > > signature for a protected method will only
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
I'm not sure how a downstream developer is supposed to know what is and is not part of our public API through casual inspection. For example, this method has no indication that the VisibleForTesting is present via the generated javadocs, https://hbase.apache.org/2.2/devapidocs/org/apache/hadoop/hbase/tool/LoadIncrementalHFiles.html#tryAtomicRegionLoad-org.apache.hadoop.hbase.client.ClientServiceCallable-org.apache.hadoop.hbase.TableName-byte:A-java.util.Collection- On Wed, Jun 24, 2020 at 2:09 PM Nick Dimiduk wrote: > On Wed, Jun 24, 2020 at 14:07 Andrew Purtell wrote: > >> I am -1 on treating VisibleForTesting as public API. Semantically it makes >> no sense. > > > By extension then you think it’s acceptable to define our public api in > terms of this annotation provided by Guava? I think that makes no sense. > > On Wed, Jun 24, 2020 at 12:22 PM Nick Dimiduk wrote: >> >> > I'd like to get this [DISCUSS] wrapped up so we can proceed with release >> > candidates. >> > >> > I don't see a clear consensus here. The conclusion I read is that >> > developers generally intended the VisibleForTesting annotation to >> indicate >> > a method is not a part of our public API, but because we don't >> explicitly >> > say this in our guide, we can't really stand on that for the community. >> > >> > I propose we take the following, conservative steps going forward: >> > >> > 1. restore any VisibleForTesting method signatures for 2.3.0, treat >> this as >> > public API going forward. >> > 2. annotate any existing methods carrying the VisibleForTesting >> annotation >> > as Deprecated in 2.3.x+, for removal in 4.0.0 >> > 3. purge the VisibleForTesting annotation from our codebase for 4.0.0, >> > involving: >> > 3a. replace VisibleForTesting with IA.Private anywhere method visibility >> > cannot be limited >> > 3b. perhaps add a new Yetus check that would ban new use of >> > VisibleForTesting >> > >> > Did I miss anything? >> > >> > Thanks, >> > Nick >> > >> > On Wed, Jun 24, 2020 at 12:22 AM Viraj Jasani >> wrote: >> > >> > > +1 to "be clear in javadoc" and to the fact that guava dependency >> just to >> > > express intention which can be done through javadoc is not >> > > required unless the library is capable of breaking compilation of >> > > downstream >> > > projects if they use VFT annotated classes/methods saying you can't >> use >> > > this >> > > (what if we have such fancy thing? :) ). >> > > >> > > >> > > On 2020/06/23 20:01:40, Sean Busbey wrote: >> > > > +1 to "do it in javadoc" unless there's some magic for IDEs brought >> > about >> > > > via the VFT annotation that I'm missing. >> > > > >> > > > On Tue, Jun 23, 2020, 13:04 Andrew Purtell >> > wrote: >> > > > >> > > > > I don't find the VisibleForTesting annotation provides a lot of >> > value. >> > > It >> > > > > became fashionable to use this annotation when a single line of >> > Javadoc >> > > > > would serve the same purpose and not make yet another dependency >> on >> > > Guava. >> > > > > My advice is to remove them all and replace with Javadoc. >> > > > > >> > > > > Even if in an IA.Public or LimitedPrivate we can decorate >> individual >> > > > > field/methods that are public but not intended to be part of the >> > public >> > > > > portion of the API with a field or method level IA.Private >> > decoration. >> > > It's >> > > > > maybe not nice to do, but that would directly and clearly express >> > > intent. >> > > > > >> > > > > On Tue, Jun 23, 2020 at 10:15 AM Sean Busbey >> > > wrote: >> > > > > >> > > > > > I think the intent behind VisibleForTesting is clear: the person >> > > using >> > > > > that >> > > > > > annotation does not intend for it to be used by downstreamers. >> > > > > > >> > > > > > However, our stated API promises are in terms of the Interface >> > > Audience >> > > > > > annotations only. So I think a downsteamer who e.g. used >> automated >> > > > > tooling >> > > > > > to ensure they only used things marked IA.Public would be >> correct >> > to >> > > be >> > > > > > upset with us if we incompatibly changed an IA.Public member >> that >> > is >> > > > > > annotated VisibleForTesting. >> > > > > > >> > > > > > Given that VisibleForTesting is in guava and we go to pains to >> > about >> > > > > > exposing downstream to non-relocated guava I think it would be a >> > bad >> > > idea >> > > > > > to use it when defining our public API. >> > > > > > >> > > > > > We should find places that use it, make sure they also carry an >> > > > > IA.Private >> > > > > > if needed, and make sure our docs for developers are clear about >> > > which >> > > > > > annotations carry meaning for downstreamers (i.e. only Interface >> > > Audience >> > > > > > and Interface Stability). >> > > > > > >> > > > > > On Tue, Jun 23, 2020, 11:29 Nick Dimiduk >> > > wrote: >> > > > > > >> > > > > > > My hope is that we can clarify our policy and update the book >> > > > > > accordingly. >> > > > > > > >> > > > > > > On Tue, Jun 23, 2020 at 9:01 AM
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
On Wed, Jun 24, 2020 at 14:07 Andrew Purtell wrote: > I am -1 on treating VisibleForTesting as public API. Semantically it makes > no sense. By extension then you think it’s acceptable to define our public api in terms of this annotation provided by Guava? I think that makes no sense. On Wed, Jun 24, 2020 at 12:22 PM Nick Dimiduk wrote: > > > I'd like to get this [DISCUSS] wrapped up so we can proceed with release > > candidates. > > > > I don't see a clear consensus here. The conclusion I read is that > > developers generally intended the VisibleForTesting annotation to > indicate > > a method is not a part of our public API, but because we don't explicitly > > say this in our guide, we can't really stand on that for the community. > > > > I propose we take the following, conservative steps going forward: > > > > 1. restore any VisibleForTesting method signatures for 2.3.0, treat this > as > > public API going forward. > > 2. annotate any existing methods carrying the VisibleForTesting > annotation > > as Deprecated in 2.3.x+, for removal in 4.0.0 > > 3. purge the VisibleForTesting annotation from our codebase for 4.0.0, > > involving: > > 3a. replace VisibleForTesting with IA.Private anywhere method visibility > > cannot be limited > > 3b. perhaps add a new Yetus check that would ban new use of > > VisibleForTesting > > > > Did I miss anything? > > > > Thanks, > > Nick > > > > On Wed, Jun 24, 2020 at 12:22 AM Viraj Jasani > wrote: > > > > > +1 to "be clear in javadoc" and to the fact that guava dependency just > to > > > express intention which can be done through javadoc is not > > > required unless the library is capable of breaking compilation of > > > downstream > > > projects if they use VFT annotated classes/methods saying you can't use > > > this > > > (what if we have such fancy thing? :) ). > > > > > > > > > On 2020/06/23 20:01:40, Sean Busbey wrote: > > > > +1 to "do it in javadoc" unless there's some magic for IDEs brought > > about > > > > via the VFT annotation that I'm missing. > > > > > > > > On Tue, Jun 23, 2020, 13:04 Andrew Purtell > > wrote: > > > > > > > > > I don't find the VisibleForTesting annotation provides a lot of > > value. > > > It > > > > > became fashionable to use this annotation when a single line of > > Javadoc > > > > > would serve the same purpose and not make yet another dependency on > > > Guava. > > > > > My advice is to remove them all and replace with Javadoc. > > > > > > > > > > Even if in an IA.Public or LimitedPrivate we can decorate > individual > > > > > field/methods that are public but not intended to be part of the > > public > > > > > portion of the API with a field or method level IA.Private > > decoration. > > > It's > > > > > maybe not nice to do, but that would directly and clearly express > > > intent. > > > > > > > > > > On Tue, Jun 23, 2020 at 10:15 AM Sean Busbey > > > wrote: > > > > > > > > > > > I think the intent behind VisibleForTesting is clear: the person > > > using > > > > > that > > > > > > annotation does not intend for it to be used by downstreamers. > > > > > > > > > > > > However, our stated API promises are in terms of the Interface > > > Audience > > > > > > annotations only. So I think a downsteamer who e.g. used > automated > > > > > tooling > > > > > > to ensure they only used things marked IA.Public would be correct > > to > > > be > > > > > > upset with us if we incompatibly changed an IA.Public member that > > is > > > > > > annotated VisibleForTesting. > > > > > > > > > > > > Given that VisibleForTesting is in guava and we go to pains to > > about > > > > > > exposing downstream to non-relocated guava I think it would be a > > bad > > > idea > > > > > > to use it when defining our public API. > > > > > > > > > > > > We should find places that use it, make sure they also carry an > > > > > IA.Private > > > > > > if needed, and make sure our docs for developers are clear about > > > which > > > > > > annotations carry meaning for downstreamers (i.e. only Interface > > > Audience > > > > > > and Interface Stability). > > > > > > > > > > > > On Tue, Jun 23, 2020, 11:29 Nick Dimiduk > > > wrote: > > > > > > > > > > > > > My hope is that we can clarify our policy and update the book > > > > > > accordingly. > > > > > > > > > > > > > > On Tue, Jun 23, 2020 at 9:01 AM Wellington Chevreuil < > > > > > > > wellington.chevre...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > For the current problem, what is the class? I think > changing > > a > > > > > method > > > > > > > > > signature for a protected method will only break the > > > compatibility > > > > > > when > > > > > > > > > users extend the class. > > > > > > > > > > > > > > > > > This specific case is > > *LoadIncrementalHFiles.tryAtomicRegionLoad, > > > > > > *mostly > > > > > > > > an end user tool, not likely to be extended. Bring back the > > > original > > > > > > > method > > > > > > > > would not be much of an issue, though, I guess the
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
I am -1 on treating VisibleForTesting as public API. Semantically it makes no sense. On Wed, Jun 24, 2020 at 12:22 PM Nick Dimiduk wrote: > I'd like to get this [DISCUSS] wrapped up so we can proceed with release > candidates. > > I don't see a clear consensus here. The conclusion I read is that > developers generally intended the VisibleForTesting annotation to indicate > a method is not a part of our public API, but because we don't explicitly > say this in our guide, we can't really stand on that for the community. > > I propose we take the following, conservative steps going forward: > > 1. restore any VisibleForTesting method signatures for 2.3.0, treat this as > public API going forward. > 2. annotate any existing methods carrying the VisibleForTesting annotation > as Deprecated in 2.3.x+, for removal in 4.0.0 > 3. purge the VisibleForTesting annotation from our codebase for 4.0.0, > involving: > 3a. replace VisibleForTesting with IA.Private anywhere method visibility > cannot be limited > 3b. perhaps add a new Yetus check that would ban new use of > VisibleForTesting > > Did I miss anything? > > Thanks, > Nick > > On Wed, Jun 24, 2020 at 12:22 AM Viraj Jasani wrote: > > > +1 to "be clear in javadoc" and to the fact that guava dependency just to > > express intention which can be done through javadoc is not > > required unless the library is capable of breaking compilation of > > downstream > > projects if they use VFT annotated classes/methods saying you can't use > > this > > (what if we have such fancy thing? :) ). > > > > > > On 2020/06/23 20:01:40, Sean Busbey wrote: > > > +1 to "do it in javadoc" unless there's some magic for IDEs brought > about > > > via the VFT annotation that I'm missing. > > > > > > On Tue, Jun 23, 2020, 13:04 Andrew Purtell > wrote: > > > > > > > I don't find the VisibleForTesting annotation provides a lot of > value. > > It > > > > became fashionable to use this annotation when a single line of > Javadoc > > > > would serve the same purpose and not make yet another dependency on > > Guava. > > > > My advice is to remove them all and replace with Javadoc. > > > > > > > > Even if in an IA.Public or LimitedPrivate we can decorate individual > > > > field/methods that are public but not intended to be part of the > public > > > > portion of the API with a field or method level IA.Private > decoration. > > It's > > > > maybe not nice to do, but that would directly and clearly express > > intent. > > > > > > > > On Tue, Jun 23, 2020 at 10:15 AM Sean Busbey > > wrote: > > > > > > > > > I think the intent behind VisibleForTesting is clear: the person > > using > > > > that > > > > > annotation does not intend for it to be used by downstreamers. > > > > > > > > > > However, our stated API promises are in terms of the Interface > > Audience > > > > > annotations only. So I think a downsteamer who e.g. used automated > > > > tooling > > > > > to ensure they only used things marked IA.Public would be correct > to > > be > > > > > upset with us if we incompatibly changed an IA.Public member that > is > > > > > annotated VisibleForTesting. > > > > > > > > > > Given that VisibleForTesting is in guava and we go to pains to > about > > > > > exposing downstream to non-relocated guava I think it would be a > bad > > idea > > > > > to use it when defining our public API. > > > > > > > > > > We should find places that use it, make sure they also carry an > > > > IA.Private > > > > > if needed, and make sure our docs for developers are clear about > > which > > > > > annotations carry meaning for downstreamers (i.e. only Interface > > Audience > > > > > and Interface Stability). > > > > > > > > > > On Tue, Jun 23, 2020, 11:29 Nick Dimiduk > > wrote: > > > > > > > > > > > My hope is that we can clarify our policy and update the book > > > > > accordingly. > > > > > > > > > > > > On Tue, Jun 23, 2020 at 9:01 AM Wellington Chevreuil < > > > > > > wellington.chevre...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > > For the current problem, what is the class? I think changing > a > > > > method > > > > > > > > signature for a protected method will only break the > > compatibility > > > > > when > > > > > > > > users extend the class. > > > > > > > > > > > > > > > This specific case is > *LoadIncrementalHFiles.tryAtomicRegionLoad, > > > > > *mostly > > > > > > > an end user tool, not likely to be extended. Bring back the > > original > > > > > > method > > > > > > > would not be much of an issue, though, I guess the discussion > is > > more > > > > > on > > > > > > > how to interpret @VisibleForTesting in general. > > > > > > > > > > > > > > Em ter., 23 de jun. de 2020 às 15:42, 张铎(Duo Zhang) < > > > > > > palomino...@gmail.com > > > > > > > > > > > > > > > escreveu: > > > > > > > > > > > > > > > Technically, I do not think the developer who makes a field > or > > > > method > > > > > > > > public for an IA.Public class but marks it with > > @VisibleForTesting, > > > > > > >
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
I'd like to get this [DISCUSS] wrapped up so we can proceed with release candidates. I don't see a clear consensus here. The conclusion I read is that developers generally intended the VisibleForTesting annotation to indicate a method is not a part of our public API, but because we don't explicitly say this in our guide, we can't really stand on that for the community. I propose we take the following, conservative steps going forward: 1. restore any VisibleForTesting method signatures for 2.3.0, treat this as public API going forward. 2. annotate any existing methods carrying the VisibleForTesting annotation as Deprecated in 2.3.x+, for removal in 4.0.0 3. purge the VisibleForTesting annotation from our codebase for 4.0.0, involving: 3a. replace VisibleForTesting with IA.Private anywhere method visibility cannot be limited 3b. perhaps add a new Yetus check that would ban new use of VisibleForTesting Did I miss anything? Thanks, Nick On Wed, Jun 24, 2020 at 12:22 AM Viraj Jasani wrote: > +1 to "be clear in javadoc" and to the fact that guava dependency just to > express intention which can be done through javadoc is not > required unless the library is capable of breaking compilation of > downstream > projects if they use VFT annotated classes/methods saying you can't use > this > (what if we have such fancy thing? :) ). > > > On 2020/06/23 20:01:40, Sean Busbey wrote: > > +1 to "do it in javadoc" unless there's some magic for IDEs brought about > > via the VFT annotation that I'm missing. > > > > On Tue, Jun 23, 2020, 13:04 Andrew Purtell wrote: > > > > > I don't find the VisibleForTesting annotation provides a lot of value. > It > > > became fashionable to use this annotation when a single line of Javadoc > > > would serve the same purpose and not make yet another dependency on > Guava. > > > My advice is to remove them all and replace with Javadoc. > > > > > > Even if in an IA.Public or LimitedPrivate we can decorate individual > > > field/methods that are public but not intended to be part of the public > > > portion of the API with a field or method level IA.Private decoration. > It's > > > maybe not nice to do, but that would directly and clearly express > intent. > > > > > > On Tue, Jun 23, 2020 at 10:15 AM Sean Busbey > wrote: > > > > > > > I think the intent behind VisibleForTesting is clear: the person > using > > > that > > > > annotation does not intend for it to be used by downstreamers. > > > > > > > > However, our stated API promises are in terms of the Interface > Audience > > > > annotations only. So I think a downsteamer who e.g. used automated > > > tooling > > > > to ensure they only used things marked IA.Public would be correct to > be > > > > upset with us if we incompatibly changed an IA.Public member that is > > > > annotated VisibleForTesting. > > > > > > > > Given that VisibleForTesting is in guava and we go to pains to about > > > > exposing downstream to non-relocated guava I think it would be a bad > idea > > > > to use it when defining our public API. > > > > > > > > We should find places that use it, make sure they also carry an > > > IA.Private > > > > if needed, and make sure our docs for developers are clear about > which > > > > annotations carry meaning for downstreamers (i.e. only Interface > Audience > > > > and Interface Stability). > > > > > > > > On Tue, Jun 23, 2020, 11:29 Nick Dimiduk > wrote: > > > > > > > > > My hope is that we can clarify our policy and update the book > > > > accordingly. > > > > > > > > > > On Tue, Jun 23, 2020 at 9:01 AM Wellington Chevreuil < > > > > > wellington.chevre...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > For the current problem, what is the class? I think changing a > > > method > > > > > > > signature for a protected method will only break the > compatibility > > > > when > > > > > > > users extend the class. > > > > > > > > > > > > > This specific case is *LoadIncrementalHFiles.tryAtomicRegionLoad, > > > > *mostly > > > > > > an end user tool, not likely to be extended. Bring back the > original > > > > > method > > > > > > would not be much of an issue, though, I guess the discussion is > more > > > > on > > > > > > how to interpret @VisibleForTesting in general. > > > > > > > > > > > > Em ter., 23 de jun. de 2020 às 15:42, 张铎(Duo Zhang) < > > > > > palomino...@gmail.com > > > > > > > > > > > > > escreveu: > > > > > > > > > > > > > Technically, I do not think the developer who makes a field or > > > method > > > > > > > public for an IA.Public class but marks it with > @VisibleForTesting, > > > > > > > actually wants to expose this field or method to end users. > > > > > > > But this could be a problem for end users, so I think we should > > > avoid > > > > > > doing > > > > > > > this on an IA.Public class in the future. > > > > > > > > > > > > > > For the current problem, what is the class? I think changing a > > > method > > > > > > > signature for a protected method will only break the >
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
+1 to "be clear in javadoc" and to the fact that guava dependency just to express intention which can be done through javadoc is not required unless the library is capable of breaking compilation of downstream projects if they use VFT annotated classes/methods saying you can't use this (what if we have such fancy thing? :) ). On 2020/06/23 20:01:40, Sean Busbey wrote: > +1 to "do it in javadoc" unless there's some magic for IDEs brought about > via the VFT annotation that I'm missing. > > On Tue, Jun 23, 2020, 13:04 Andrew Purtell wrote: > > > I don't find the VisibleForTesting annotation provides a lot of value. It > > became fashionable to use this annotation when a single line of Javadoc > > would serve the same purpose and not make yet another dependency on Guava. > > My advice is to remove them all and replace with Javadoc. > > > > Even if in an IA.Public or LimitedPrivate we can decorate individual > > field/methods that are public but not intended to be part of the public > > portion of the API with a field or method level IA.Private decoration. It's > > maybe not nice to do, but that would directly and clearly express intent. > > > > On Tue, Jun 23, 2020 at 10:15 AM Sean Busbey wrote: > > > > > I think the intent behind VisibleForTesting is clear: the person using > > that > > > annotation does not intend for it to be used by downstreamers. > > > > > > However, our stated API promises are in terms of the Interface Audience > > > annotations only. So I think a downsteamer who e.g. used automated > > tooling > > > to ensure they only used things marked IA.Public would be correct to be > > > upset with us if we incompatibly changed an IA.Public member that is > > > annotated VisibleForTesting. > > > > > > Given that VisibleForTesting is in guava and we go to pains to about > > > exposing downstream to non-relocated guava I think it would be a bad idea > > > to use it when defining our public API. > > > > > > We should find places that use it, make sure they also carry an > > IA.Private > > > if needed, and make sure our docs for developers are clear about which > > > annotations carry meaning for downstreamers (i.e. only Interface Audience > > > and Interface Stability). > > > > > > On Tue, Jun 23, 2020, 11:29 Nick Dimiduk wrote: > > > > > > > My hope is that we can clarify our policy and update the book > > > accordingly. > > > > > > > > On Tue, Jun 23, 2020 at 9:01 AM Wellington Chevreuil < > > > > wellington.chevre...@gmail.com> wrote: > > > > > > > > > > > > > > > > For the current problem, what is the class? I think changing a > > method > > > > > > signature for a protected method will only break the compatibility > > > when > > > > > > users extend the class. > > > > > > > > > > > This specific case is *LoadIncrementalHFiles.tryAtomicRegionLoad, > > > *mostly > > > > > an end user tool, not likely to be extended. Bring back the original > > > > method > > > > > would not be much of an issue, though, I guess the discussion is more > > > on > > > > > how to interpret @VisibleForTesting in general. > > > > > > > > > > Em ter., 23 de jun. de 2020 às 15:42, 张铎(Duo Zhang) < > > > > palomino...@gmail.com > > > > > > > > > > > escreveu: > > > > > > > > > > > Technically, I do not think the developer who makes a field or > > method > > > > > > public for an IA.Public class but marks it with @VisibleForTesting, > > > > > > actually wants to expose this field or method to end users. > > > > > > But this could be a problem for end users, so I think we should > > avoid > > > > > doing > > > > > > this on an IA.Public class in the future. > > > > > > > > > > > > For the current problem, what is the class? I think changing a > > method > > > > > > signature for a protected method will only break the compatibility > > > when > > > > > > users extend the class. In fact, most of the classes in our public > > > API > > > > > are > > > > > > not designed to be extended by end users. > > > > > > > > > > > > Thanks. > > > > > > > > > > > > Wellington Chevreuil > > 于2020年6月23日周二 > > > > > > 下午10:33写道： > > > > > > > > > > > > > My opinion expressed on the 2.3.0RC0 thread was that > > > > @VisibleForTesting > > > > > > > would flag class/method/variable as private. I believe the > > > annotation > > > > > > label > > > > > > > is pretty suggestive and (I also believe) it's common sense that > > it > > > > > > should > > > > > > > be treated as private by developers. I don't think the fact it's > > > > > > > omitted from our guidelines changes perception of it. > > > > > > > > > > > > > > Em ter., 23 de jun. de 2020 às 01:15, Bharath Vissapragada < > > > > > > > bhara...@apache.org> escreveu: > > > > > > > > > > > > > > > Sorry, I should've been clearer. It's the former. My point is, > > > any > > > > > > method > > > > > > > > tagged with @VisibleForTesting is only intended for testing > > > > purposes > > > > > > and > > > > > > > > should _not_ be considered public, its visibility scope is > > wider > > >
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
+1 to "do it in javadoc" unless there's some magic for IDEs brought about via the VFT annotation that I'm missing. On Tue, Jun 23, 2020, 13:04 Andrew Purtell wrote: > I don't find the VisibleForTesting annotation provides a lot of value. It > became fashionable to use this annotation when a single line of Javadoc > would serve the same purpose and not make yet another dependency on Guava. > My advice is to remove them all and replace with Javadoc. > > Even if in an IA.Public or LimitedPrivate we can decorate individual > field/methods that are public but not intended to be part of the public > portion of the API with a field or method level IA.Private decoration. It's > maybe not nice to do, but that would directly and clearly express intent. > > On Tue, Jun 23, 2020 at 10:15 AM Sean Busbey wrote: > > > I think the intent behind VisibleForTesting is clear: the person using > that > > annotation does not intend for it to be used by downstreamers. > > > > However, our stated API promises are in terms of the Interface Audience > > annotations only. So I think a downsteamer who e.g. used automated > tooling > > to ensure they only used things marked IA.Public would be correct to be > > upset with us if we incompatibly changed an IA.Public member that is > > annotated VisibleForTesting. > > > > Given that VisibleForTesting is in guava and we go to pains to about > > exposing downstream to non-relocated guava I think it would be a bad idea > > to use it when defining our public API. > > > > We should find places that use it, make sure they also carry an > IA.Private > > if needed, and make sure our docs for developers are clear about which > > annotations carry meaning for downstreamers (i.e. only Interface Audience > > and Interface Stability). > > > > On Tue, Jun 23, 2020, 11:29 Nick Dimiduk wrote: > > > > > My hope is that we can clarify our policy and update the book > > accordingly. > > > > > > On Tue, Jun 23, 2020 at 9:01 AM Wellington Chevreuil < > > > wellington.chevre...@gmail.com> wrote: > > > > > > > > > > > > > For the current problem, what is the class? I think changing a > method > > > > > signature for a protected method will only break the compatibility > > when > > > > > users extend the class. > > > > > > > > > This specific case is *LoadIncrementalHFiles.tryAtomicRegionLoad, > > *mostly > > > > an end user tool, not likely to be extended. Bring back the original > > > method > > > > would not be much of an issue, though, I guess the discussion is more > > on > > > > how to interpret @VisibleForTesting in general. > > > > > > > > Em ter., 23 de jun. de 2020 às 15:42, 张铎(Duo Zhang) < > > > palomino...@gmail.com > > > > > > > > > escreveu: > > > > > > > > > Technically, I do not think the developer who makes a field or > method > > > > > public for an IA.Public class but marks it with @VisibleForTesting, > > > > > actually wants to expose this field or method to end users. > > > > > But this could be a problem for end users, so I think we should > avoid > > > > doing > > > > > this on an IA.Public class in the future. > > > > > > > > > > For the current problem, what is the class? I think changing a > method > > > > > signature for a protected method will only break the compatibility > > when > > > > > users extend the class. In fact, most of the classes in our public > > API > > > > are > > > > > not designed to be extended by end users. > > > > > > > > > > Thanks. > > > > > > > > > > Wellington Chevreuil > 于2020年6月23日周二 > > > > > 下午10:33写道： > > > > > > > > > > > My opinion expressed on the 2.3.0RC0 thread was that > > > @VisibleForTesting > > > > > > would flag class/method/variable as private. I believe the > > annotation > > > > > label > > > > > > is pretty suggestive and (I also believe) it's common sense that > it > > > > > should > > > > > > be treated as private by developers. I don't think the fact it's > > > > > > omitted from our guidelines changes perception of it. > > > > > > > > > > > > Em ter., 23 de jun. de 2020 às 01:15, Bharath Vissapragada < > > > > > > bhara...@apache.org> escreveu: > > > > > > > > > > > > > Sorry, I should've been clearer. It's the former. My point is, > > any > > > > > method > > > > > > > tagged with @VisibleForTesting is only intended for testing > > > purposes > > > > > and > > > > > > > should _not_ be considered public, its visibility scope is > wider > > > than > > > > > > > necessary only because it was needed by some test method. > That's > > > how > > > > > I'd > > > > > > > interpret it (Actually, that's what I thought you meant, now > I'm > > > > > confused > > > > > > > :-)). > > > > > > > > > > > > > > On Mon, Jun 22, 2020 at 4:02 PM Nick Dimiduk < > > ndimi...@apache.org> > > > > > > wrote: > > > > > > > > > > > > > > > On Mon, Jun 22, 2020 at 3:45 PM Bharath Vissapragada < > > > > > > > bhara...@apache.org> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > I share the same opinion. Infact hadoop (from which our > > > >
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
I don't find the VisibleForTesting annotation provides a lot of value. It became fashionable to use this annotation when a single line of Javadoc would serve the same purpose and not make yet another dependency on Guava. My advice is to remove them all and replace with Javadoc. Even if in an IA.Public or LimitedPrivate we can decorate individual field/methods that are public but not intended to be part of the public portion of the API with a field or method level IA.Private decoration. It's maybe not nice to do, but that would directly and clearly express intent. On Tue, Jun 23, 2020 at 10:15 AM Sean Busbey wrote: > I think the intent behind VisibleForTesting is clear: the person using that > annotation does not intend for it to be used by downstreamers. > > However, our stated API promises are in terms of the Interface Audience > annotations only. So I think a downsteamer who e.g. used automated tooling > to ensure they only used things marked IA.Public would be correct to be > upset with us if we incompatibly changed an IA.Public member that is > annotated VisibleForTesting. > > Given that VisibleForTesting is in guava and we go to pains to about > exposing downstream to non-relocated guava I think it would be a bad idea > to use it when defining our public API. > > We should find places that use it, make sure they also carry an IA.Private > if needed, and make sure our docs for developers are clear about which > annotations carry meaning for downstreamers (i.e. only Interface Audience > and Interface Stability). > > On Tue, Jun 23, 2020, 11:29 Nick Dimiduk wrote: > > > My hope is that we can clarify our policy and update the book > accordingly. > > > > On Tue, Jun 23, 2020 at 9:01 AM Wellington Chevreuil < > > wellington.chevre...@gmail.com> wrote: > > > > > > > > > > For the current problem, what is the class? I think changing a method > > > > signature for a protected method will only break the compatibility > when > > > > users extend the class. > > > > > > > This specific case is *LoadIncrementalHFiles.tryAtomicRegionLoad, > *mostly > > > an end user tool, not likely to be extended. Bring back the original > > method > > > would not be much of an issue, though, I guess the discussion is more > on > > > how to interpret @VisibleForTesting in general. > > > > > > Em ter., 23 de jun. de 2020 às 15:42, 张铎(Duo Zhang) < > > palomino...@gmail.com > > > > > > > escreveu: > > > > > > > Technically, I do not think the developer who makes a field or method > > > > public for an IA.Public class but marks it with @VisibleForTesting, > > > > actually wants to expose this field or method to end users. > > > > But this could be a problem for end users, so I think we should avoid > > > doing > > > > this on an IA.Public class in the future. > > > > > > > > For the current problem, what is the class? I think changing a method > > > > signature for a protected method will only break the compatibility > when > > > > users extend the class. In fact, most of the classes in our public > API > > > are > > > > not designed to be extended by end users. > > > > > > > > Thanks. > > > > > > > > Wellington Chevreuil 于2020年6月23日周二 > > > > 下午10:33写道： > > > > > > > > > My opinion expressed on the 2.3.0RC0 thread was that > > @VisibleForTesting > > > > > would flag class/method/variable as private. I believe the > annotation > > > > label > > > > > is pretty suggestive and (I also believe) it's common sense that it > > > > should > > > > > be treated as private by developers. I don't think the fact it's > > > > > omitted from our guidelines changes perception of it. > > > > > > > > > > Em ter., 23 de jun. de 2020 às 01:15, Bharath Vissapragada < > > > > > bhara...@apache.org> escreveu: > > > > > > > > > > > Sorry, I should've been clearer. It's the former. My point is, > any > > > > method > > > > > > tagged with @VisibleForTesting is only intended for testing > > purposes > > > > and > > > > > > should _not_ be considered public, its visibility scope is wider > > than > > > > > > necessary only because it was needed by some test method. That's > > how > > > > I'd > > > > > > interpret it (Actually, that's what I thought you meant, now I'm > > > > confused > > > > > > :-)). > > > > > > > > > > > > On Mon, Jun 22, 2020 at 4:02 PM Nick Dimiduk < > ndimi...@apache.org> > > > > > wrote: > > > > > > > > > > > > > On Mon, Jun 22, 2020 at 3:45 PM Bharath Vissapragada < > > > > > > bhara...@apache.org> > > > > > > > wrote: > > > > > > > > > > > > > > > I share the same opinion. Infact hadoop (from which our > > > annotations > > > > > are > > > > > > > > derived I believe), talks about this, "Also, certain APIs are > > > > > annotated > > > > > > > as > > > > > > > > @VisibleForTesting (from com.google.common > > > > > > > .annotations.VisibleForTesting) > > > > > > > > - these are meant to be used strictly for unit tests and > should > > > be > > > > > > > treated > > > > > > > > as “Private” APIs." > > > > > > > > > > > > > > > > >
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
I think the intent behind VisibleForTesting is clear: the person using that annotation does not intend for it to be used by downstreamers. However, our stated API promises are in terms of the Interface Audience annotations only. So I think a downsteamer who e.g. used automated tooling to ensure they only used things marked IA.Public would be correct to be upset with us if we incompatibly changed an IA.Public member that is annotated VisibleForTesting. Given that VisibleForTesting is in guava and we go to pains to about exposing downstream to non-relocated guava I think it would be a bad idea to use it when defining our public API. We should find places that use it, make sure they also carry an IA.Private if needed, and make sure our docs for developers are clear about which annotations carry meaning for downstreamers (i.e. only Interface Audience and Interface Stability). On Tue, Jun 23, 2020, 11:29 Nick Dimiduk wrote: > My hope is that we can clarify our policy and update the book accordingly. > > On Tue, Jun 23, 2020 at 9:01 AM Wellington Chevreuil < > wellington.chevre...@gmail.com> wrote: > > > > > > > For the current problem, what is the class? I think changing a method > > > signature for a protected method will only break the compatibility when > > > users extend the class. > > > > > This specific case is *LoadIncrementalHFiles.tryAtomicRegionLoad, *mostly > > an end user tool, not likely to be extended. Bring back the original > method > > would not be much of an issue, though, I guess the discussion is more on > > how to interpret @VisibleForTesting in general. > > > > Em ter., 23 de jun. de 2020 às 15:42, 张铎(Duo Zhang) < > palomino...@gmail.com > > > > > escreveu: > > > > > Technically, I do not think the developer who makes a field or method > > > public for an IA.Public class but marks it with @VisibleForTesting, > > > actually wants to expose this field or method to end users. > > > But this could be a problem for end users, so I think we should avoid > > doing > > > this on an IA.Public class in the future. > > > > > > For the current problem, what is the class? I think changing a method > > > signature for a protected method will only break the compatibility when > > > users extend the class. In fact, most of the classes in our public API > > are > > > not designed to be extended by end users. > > > > > > Thanks. > > > > > > Wellington Chevreuil 于2020年6月23日周二 > > > 下午10:33写道： > > > > > > > My opinion expressed on the 2.3.0RC0 thread was that > @VisibleForTesting > > > > would flag class/method/variable as private. I believe the annotation > > > label > > > > is pretty suggestive and (I also believe) it's common sense that it > > > should > > > > be treated as private by developers. I don't think the fact it's > > > > omitted from our guidelines changes perception of it. > > > > > > > > Em ter., 23 de jun. de 2020 às 01:15, Bharath Vissapragada < > > > > bhara...@apache.org> escreveu: > > > > > > > > > Sorry, I should've been clearer. It's the former. My point is, any > > > method > > > > > tagged with @VisibleForTesting is only intended for testing > purposes > > > and > > > > > should _not_ be considered public, its visibility scope is wider > than > > > > > necessary only because it was needed by some test method. That's > how > > > I'd > > > > > interpret it (Actually, that's what I thought you meant, now I'm > > > confused > > > > > :-)). > > > > > > > > > > On Mon, Jun 22, 2020 at 4:02 PM Nick Dimiduk > > > > wrote: > > > > > > > > > > > On Mon, Jun 22, 2020 at 3:45 PM Bharath Vissapragada < > > > > > bhara...@apache.org> > > > > > > wrote: > > > > > > > > > > > > > I share the same opinion. Infact hadoop (from which our > > annotations > > > > are > > > > > > > derived I believe), talks about this, "Also, certain APIs are > > > > annotated > > > > > > as > > > > > > > @VisibleForTesting (from com.google.common > > > > > > .annotations.VisibleForTesting) > > > > > > > - these are meant to be used strictly for unit tests and should > > be > > > > > > treated > > > > > > > as “Private” APIs." > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://hadoop.apache.org/docs/r3.1.2/hadoop-project-dist/hadoop-common/InterfaceClassification.html > > > > > > > > > > > > > > > > > > > Sorry Bharath, I don't follow. Are you saying "I share the > opinion > > > that > > > > > the > > > > > > VisibleForTesting annotation should be considered as defining a > > > method > > > > as > > > > > > IA.Private," and this is an omission from our community > guidelines > > > > > > document? Or are you saying "no, it does not count as an > interface > > > > > audience > > > > > > marker," and we are obliged to treat methods such as in this > > example > > > as > > > > > > public API? > > > > > > > > > > > > Thanks, > > > > > > Nick > > > > > > > > > > > > On Mon, Jun 22, 2020 at 10:15 AM Sean Busbey > > > > wrote: > > > > > > > > > > > > > > > Yeah I would say no as
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
My hope is that we can clarify our policy and update the book accordingly. On Tue, Jun 23, 2020 at 9:01 AM Wellington Chevreuil < wellington.chevre...@gmail.com> wrote: > > > > For the current problem, what is the class? I think changing a method > > signature for a protected method will only break the compatibility when > > users extend the class. > > > This specific case is *LoadIncrementalHFiles.tryAtomicRegionLoad, *mostly > an end user tool, not likely to be extended. Bring back the original method > would not be much of an issue, though, I guess the discussion is more on > how to interpret @VisibleForTesting in general. > > Em ter., 23 de jun. de 2020 às 15:42, 张铎(Duo Zhang) > > escreveu: > > > Technically, I do not think the developer who makes a field or method > > public for an IA.Public class but marks it with @VisibleForTesting, > > actually wants to expose this field or method to end users. > > But this could be a problem for end users, so I think we should avoid > doing > > this on an IA.Public class in the future. > > > > For the current problem, what is the class? I think changing a method > > signature for a protected method will only break the compatibility when > > users extend the class. In fact, most of the classes in our public API > are > > not designed to be extended by end users. > > > > Thanks. > > > > Wellington Chevreuil 于2020年6月23日周二 > > 下午10:33写道： > > > > > My opinion expressed on the 2.3.0RC0 thread was that @VisibleForTesting > > > would flag class/method/variable as private. I believe the annotation > > label > > > is pretty suggestive and (I also believe) it's common sense that it > > should > > > be treated as private by developers. I don't think the fact it's > > > omitted from our guidelines changes perception of it. > > > > > > Em ter., 23 de jun. de 2020 às 01:15, Bharath Vissapragada < > > > bhara...@apache.org> escreveu: > > > > > > > Sorry, I should've been clearer. It's the former. My point is, any > > method > > > > tagged with @VisibleForTesting is only intended for testing purposes > > and > > > > should _not_ be considered public, its visibility scope is wider than > > > > necessary only because it was needed by some test method. That's how > > I'd > > > > interpret it (Actually, that's what I thought you meant, now I'm > > confused > > > > :-)). > > > > > > > > On Mon, Jun 22, 2020 at 4:02 PM Nick Dimiduk > > > wrote: > > > > > > > > > On Mon, Jun 22, 2020 at 3:45 PM Bharath Vissapragada < > > > > bhara...@apache.org> > > > > > wrote: > > > > > > > > > > > I share the same opinion. Infact hadoop (from which our > annotations > > > are > > > > > > derived I believe), talks about this, "Also, certain APIs are > > > annotated > > > > > as > > > > > > @VisibleForTesting (from com.google.common > > > > > .annotations.VisibleForTesting) > > > > > > - these are meant to be used strictly for unit tests and should > be > > > > > treated > > > > > > as “Private” APIs." > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://hadoop.apache.org/docs/r3.1.2/hadoop-project-dist/hadoop-common/InterfaceClassification.html > > > > > > > > > > > > > > > > Sorry Bharath, I don't follow. Are you saying "I share the opinion > > that > > > > the > > > > > VisibleForTesting annotation should be considered as defining a > > method > > > as > > > > > IA.Private," and this is an omission from our community guidelines > > > > > document? Or are you saying "no, it does not count as an interface > > > > audience > > > > > marker," and we are obliged to treat methods such as in this > example > > as > > > > > public API? > > > > > > > > > > Thanks, > > > > > Nick > > > > > > > > > > On Mon, Jun 22, 2020 at 10:15 AM Sean Busbey > > > wrote: > > > > > > > > > > > > > Yeah I would say no as well. We should make clear on our dev > > guide > > > > that > > > > > > you > > > > > > > also should be marking those things with an Interface Audience > > > > marking > > > > > if > > > > > > > you don't intend them to be at the downstream API visibility of > > the > > > > > > parent > > > > > > > class. > > > > > > > > > > > > > > (IIRC we also use VisibleForTesting in IA.Private classes to > > > > > proactively > > > > > > > explain why some internal looking member is at a wider Java > > access > > > > > > scope.) > > > > > > > > > > > > > > On Mon, Jun 22, 2020, 11:39 Nick Dimiduk > > > > wrote: > > > > > > > > > > > > > > > Hello, > > > > > > > > > > > > > > > > This came up over on the 2.3.0RC0 thread, so let's open it > for > > > > proper > > > > > > > > discussion. In that context, we observe method signature > > changes > > > > to a > > > > > > > > method marked with the Guava VisibleForTesting annotation. > The > > > > method > > > > > > is > > > > > > > a > > > > > > > > protected method on a IA.Public class. There is no > method-level > > > IA > > > > > > > > annotation. > > > > > > > > > > > > > > > > Do we consider the VisibleForTesting annotation as a > specifier >
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
> > For the current problem, what is the class? I think changing a method > signature for a protected method will only break the compatibility when > users extend the class. > This specific case is *LoadIncrementalHFiles.tryAtomicRegionLoad, *mostly an end user tool, not likely to be extended. Bring back the original method would not be much of an issue, though, I guess the discussion is more on how to interpret @VisibleForTesting in general. Em ter., 23 de jun. de 2020 às 15:42, 张铎(Duo Zhang) escreveu: > Technically, I do not think the developer who makes a field or method > public for an IA.Public class but marks it with @VisibleForTesting, > actually wants to expose this field or method to end users. > But this could be a problem for end users, so I think we should avoid doing > this on an IA.Public class in the future. > > For the current problem, what is the class? I think changing a method > signature for a protected method will only break the compatibility when > users extend the class. In fact, most of the classes in our public API are > not designed to be extended by end users. > > Thanks. > > Wellington Chevreuil 于2020年6月23日周二 > 下午10:33写道： > > > My opinion expressed on the 2.3.0RC0 thread was that @VisibleForTesting > > would flag class/method/variable as private. I believe the annotation > label > > is pretty suggestive and (I also believe) it's common sense that it > should > > be treated as private by developers. I don't think the fact it's > > omitted from our guidelines changes perception of it. > > > > Em ter., 23 de jun. de 2020 às 01:15, Bharath Vissapragada < > > bhara...@apache.org> escreveu: > > > > > Sorry, I should've been clearer. It's the former. My point is, any > method > > > tagged with @VisibleForTesting is only intended for testing purposes > and > > > should _not_ be considered public, its visibility scope is wider than > > > necessary only because it was needed by some test method. That's how > I'd > > > interpret it (Actually, that's what I thought you meant, now I'm > confused > > > :-)). > > > > > > On Mon, Jun 22, 2020 at 4:02 PM Nick Dimiduk > > wrote: > > > > > > > On Mon, Jun 22, 2020 at 3:45 PM Bharath Vissapragada < > > > bhara...@apache.org> > > > > wrote: > > > > > > > > > I share the same opinion. Infact hadoop (from which our annotations > > are > > > > > derived I believe), talks about this, "Also, certain APIs are > > annotated > > > > as > > > > > @VisibleForTesting (from com.google.common > > > > .annotations.VisibleForTesting) > > > > > - these are meant to be used strictly for unit tests and should be > > > > treated > > > > > as “Private” APIs." > > > > > > > > > > > > > > > > > > > > > > > > > https://hadoop.apache.org/docs/r3.1.2/hadoop-project-dist/hadoop-common/InterfaceClassification.html > > > > > > > > > > > > > Sorry Bharath, I don't follow. Are you saying "I share the opinion > that > > > the > > > > VisibleForTesting annotation should be considered as defining a > method > > as > > > > IA.Private," and this is an omission from our community guidelines > > > > document? Or are you saying "no, it does not count as an interface > > > audience > > > > marker," and we are obliged to treat methods such as in this example > as > > > > public API? > > > > > > > > Thanks, > > > > Nick > > > > > > > > On Mon, Jun 22, 2020 at 10:15 AM Sean Busbey > > wrote: > > > > > > > > > > > Yeah I would say no as well. We should make clear on our dev > guide > > > that > > > > > you > > > > > > also should be marking those things with an Interface Audience > > > marking > > > > if > > > > > > you don't intend them to be at the downstream API visibility of > the > > > > > parent > > > > > > class. > > > > > > > > > > > > (IIRC we also use VisibleForTesting in IA.Private classes to > > > > proactively > > > > > > explain why some internal looking member is at a wider Java > access > > > > > scope.) > > > > > > > > > > > > On Mon, Jun 22, 2020, 11:39 Nick Dimiduk > > > wrote: > > > > > > > > > > > > > Hello, > > > > > > > > > > > > > > This came up over on the 2.3.0RC0 thread, so let's open it for > > > proper > > > > > > > discussion. In that context, we observe method signature > changes > > > to a > > > > > > > method marked with the Guava VisibleForTesting annotation. The > > > method > > > > > is > > > > > > a > > > > > > > protected method on a IA.Public class. There is no method-level > > IA > > > > > > > annotation. > > > > > > > > > > > > > > Do we consider the VisibleForTesting annotation as a specifier > > for > > > > our > > > > > > > compatibility guidelines? > > > > > > > > > > > > > > I am of the opinion that no, it is not an InterfaceAudience > > > > annotation, > > > > > > and > > > > > > > so it is not applicable for defining our public API. > > > > > > > > > > > > > > What do you think? > > > > > > > > > > > > > > Thanks, > > > > > > > Nick > > > > > > > > > > > > > > > > > > > > > > > > > > > >
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
Technically, I do not think the developer who makes a field or method public for an IA.Public class but marks it with @VisibleForTesting, actually wants to expose this field or method to end users. But this could be a problem for end users, so I think we should avoid doing this on an IA.Public class in the future. For the current problem, what is the class? I think changing a method signature for a protected method will only break the compatibility when users extend the class. In fact, most of the classes in our public API are not designed to be extended by end users. Thanks. Wellington Chevreuil 于2020年6月23日周二 下午10:33写道： > My opinion expressed on the 2.3.0RC0 thread was that @VisibleForTesting > would flag class/method/variable as private. I believe the annotation label > is pretty suggestive and (I also believe) it's common sense that it should > be treated as private by developers. I don't think the fact it's > omitted from our guidelines changes perception of it. > > Em ter., 23 de jun. de 2020 às 01:15, Bharath Vissapragada < > bhara...@apache.org> escreveu: > > > Sorry, I should've been clearer. It's the former. My point is, any method > > tagged with @VisibleForTesting is only intended for testing purposes and > > should _not_ be considered public, its visibility scope is wider than > > necessary only because it was needed by some test method. That's how I'd > > interpret it (Actually, that's what I thought you meant, now I'm confused > > :-)). > > > > On Mon, Jun 22, 2020 at 4:02 PM Nick Dimiduk > wrote: > > > > > On Mon, Jun 22, 2020 at 3:45 PM Bharath Vissapragada < > > bhara...@apache.org> > > > wrote: > > > > > > > I share the same opinion. Infact hadoop (from which our annotations > are > > > > derived I believe), talks about this, "Also, certain APIs are > annotated > > > as > > > > @VisibleForTesting (from com.google.common > > > .annotations.VisibleForTesting) > > > > - these are meant to be used strictly for unit tests and should be > > > treated > > > > as “Private” APIs." > > > > > > > > > > > > > > > > > > https://hadoop.apache.org/docs/r3.1.2/hadoop-project-dist/hadoop-common/InterfaceClassification.html > > > > > > > > > > Sorry Bharath, I don't follow. Are you saying "I share the opinion that > > the > > > VisibleForTesting annotation should be considered as defining a method > as > > > IA.Private," and this is an omission from our community guidelines > > > document? Or are you saying "no, it does not count as an interface > > audience > > > marker," and we are obliged to treat methods such as in this example as > > > public API? > > > > > > Thanks, > > > Nick > > > > > > On Mon, Jun 22, 2020 at 10:15 AM Sean Busbey > wrote: > > > > > > > > > Yeah I would say no as well. We should make clear on our dev guide > > that > > > > you > > > > > also should be marking those things with an Interface Audience > > marking > > > if > > > > > you don't intend them to be at the downstream API visibility of the > > > > parent > > > > > class. > > > > > > > > > > (IIRC we also use VisibleForTesting in IA.Private classes to > > > proactively > > > > > explain why some internal looking member is at a wider Java access > > > > scope.) > > > > > > > > > > On Mon, Jun 22, 2020, 11:39 Nick Dimiduk > > wrote: > > > > > > > > > > > Hello, > > > > > > > > > > > > This came up over on the 2.3.0RC0 thread, so let's open it for > > proper > > > > > > discussion. In that context, we observe method signature changes > > to a > > > > > > method marked with the Guava VisibleForTesting annotation. The > > method > > > > is > > > > > a > > > > > > protected method on a IA.Public class. There is no method-level > IA > > > > > > annotation. > > > > > > > > > > > > Do we consider the VisibleForTesting annotation as a specifier > for > > > our > > > > > > compatibility guidelines? > > > > > > > > > > > > I am of the opinion that no, it is not an InterfaceAudience > > > annotation, > > > > > and > > > > > > so it is not applicable for defining our public API. > > > > > > > > > > > > What do you think? > > > > > > > > > > > > Thanks, > > > > > > Nick > > > > > > > > > > > > > > > > > > > > >
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
My opinion expressed on the 2.3.0RC0 thread was that @VisibleForTesting would flag class/method/variable as private. I believe the annotation label is pretty suggestive and (I also believe) it's common sense that it should be treated as private by developers. I don't think the fact it's omitted from our guidelines changes perception of it. Em ter., 23 de jun. de 2020 às 01:15, Bharath Vissapragada < bhara...@apache.org> escreveu: > Sorry, I should've been clearer. It's the former. My point is, any method > tagged with @VisibleForTesting is only intended for testing purposes and > should _not_ be considered public, its visibility scope is wider than > necessary only because it was needed by some test method. That's how I'd > interpret it (Actually, that's what I thought you meant, now I'm confused > :-)). > > On Mon, Jun 22, 2020 at 4:02 PM Nick Dimiduk wrote: > > > On Mon, Jun 22, 2020 at 3:45 PM Bharath Vissapragada < > bhara...@apache.org> > > wrote: > > > > > I share the same opinion. Infact hadoop (from which our annotations are > > > derived I believe), talks about this, "Also, certain APIs are annotated > > as > > > @VisibleForTesting (from com.google.common > > .annotations.VisibleForTesting) > > > - these are meant to be used strictly for unit tests and should be > > treated > > > as “Private” APIs." > > > > > > > > > > > > https://hadoop.apache.org/docs/r3.1.2/hadoop-project-dist/hadoop-common/InterfaceClassification.html > > > > > > > Sorry Bharath, I don't follow. Are you saying "I share the opinion that > the > > VisibleForTesting annotation should be considered as defining a method as > > IA.Private," and this is an omission from our community guidelines > > document? Or are you saying "no, it does not count as an interface > audience > > marker," and we are obliged to treat methods such as in this example as > > public API? > > > > Thanks, > > Nick > > > > On Mon, Jun 22, 2020 at 10:15 AM Sean Busbey wrote: > > > > > > > Yeah I would say no as well. We should make clear on our dev guide > that > > > you > > > > also should be marking those things with an Interface Audience > marking > > if > > > > you don't intend them to be at the downstream API visibility of the > > > parent > > > > class. > > > > > > > > (IIRC we also use VisibleForTesting in IA.Private classes to > > proactively > > > > explain why some internal looking member is at a wider Java access > > > scope.) > > > > > > > > On Mon, Jun 22, 2020, 11:39 Nick Dimiduk > wrote: > > > > > > > > > Hello, > > > > > > > > > > This came up over on the 2.3.0RC0 thread, so let's open it for > proper > > > > > discussion. In that context, we observe method signature changes > to a > > > > > method marked with the Guava VisibleForTesting annotation. The > method > > > is > > > > a > > > > > protected method on a IA.Public class. There is no method-level IA > > > > > annotation. > > > > > > > > > > Do we consider the VisibleForTesting annotation as a specifier for > > our > > > > > compatibility guidelines? > > > > > > > > > > I am of the opinion that no, it is not an InterfaceAudience > > annotation, > > > > and > > > > > so it is not applicable for defining our public API. > > > > > > > > > > What do you think? > > > > > > > > > > Thanks, > > > > > Nick > > > > > > > > > > > > > > >
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
I also share the former opinion from two opinions provided by Nick. And I also thought that Nick and Sean were of the same opinion that @VisibleForTesting is only intended for our project and should not be treated as public API by downstreamers. After Bharath's reply, I again went through the thread and now I feel I was incorrect. Sean and Nick are of the opinion that @VisibleForTesting with wider scope (although intended for our unit tests) should be treated as public API and we should follow deprecation cycle for minor release for them. Is that correct? IMHO these methods should be strictly used by tests only and downstreamers should treat them as upstream's private APIs and should never use them directly. On 2020/06/23 00:15:39, Bharath Vissapragada wrote: > Sorry, I should've been clearer. It's the former. My point is, any method > tagged with @VisibleForTesting is only intended for testing purposes and > should _not_ be considered public, its visibility scope is wider than > necessary only because it was needed by some test method. That's how I'd > interpret it (Actually, that's what I thought you meant, now I'm confused > :-)). > > On Mon, Jun 22, 2020 at 4:02 PM Nick Dimiduk wrote: > > > On Mon, Jun 22, 2020 at 3:45 PM Bharath Vissapragada > > wrote: > > > > > I share the same opinion. Infact hadoop (from which our annotations are > > > derived I believe), talks about this, "Also, certain APIs are annotated > > as > > > @VisibleForTesting (from com.google.common > > .annotations.VisibleForTesting) > > > - these are meant to be used strictly for unit tests and should be > > treated > > > as “Private” APIs." > > > > > > > > > > > https://hadoop.apache.org/docs/r3.1.2/hadoop-project-dist/hadoop-common/InterfaceClassification.html > > > > > > > Sorry Bharath, I don't follow. Are you saying "I share the opinion that the > > VisibleForTesting annotation should be considered as defining a method as > > IA.Private," and this is an omission from our community guidelines > > document? Or are you saying "no, it does not count as an interface audience > > marker," and we are obliged to treat methods such as in this example as > > public API? > > > > Thanks, > > Nick > > > > On Mon, Jun 22, 2020 at 10:15 AM Sean Busbey wrote: > > > > > > > Yeah I would say no as well. We should make clear on our dev guide that > > > you > > > > also should be marking those things with an Interface Audience marking > > if > > > > you don't intend them to be at the downstream API visibility of the > > > parent > > > > class. > > > > > > > > (IIRC we also use VisibleForTesting in IA.Private classes to > > proactively > > > > explain why some internal looking member is at a wider Java access > > > scope.) > > > > > > > > On Mon, Jun 22, 2020, 11:39 Nick Dimiduk wrote: > > > > > > > > > Hello, > > > > > > > > > > This came up over on the 2.3.0RC0 thread, so let's open it for proper > > > > > discussion. In that context, we observe method signature changes to a > > > > > method marked with the Guava VisibleForTesting annotation. The method > > > is > > > > a > > > > > protected method on a IA.Public class. There is no method-level IA > > > > > annotation. > > > > > > > > > > Do we consider the VisibleForTesting annotation as a specifier for > > our > > > > > compatibility guidelines? > > > > > > > > > > I am of the opinion that no, it is not an InterfaceAudience > > annotation, > > > > and > > > > > so it is not applicable for defining our public API. > > > > > > > > > > What do you think? > > > > > > > > > > Thanks, > > > > > Nick > > > > > > > > > > > > > > >
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
Sorry, I should've been clearer. It's the former. My point is, any method tagged with @VisibleForTesting is only intended for testing purposes and should _not_ be considered public, its visibility scope is wider than necessary only because it was needed by some test method. That's how I'd interpret it (Actually, that's what I thought you meant, now I'm confused :-)). On Mon, Jun 22, 2020 at 4:02 PM Nick Dimiduk wrote: > On Mon, Jun 22, 2020 at 3:45 PM Bharath Vissapragada > wrote: > > > I share the same opinion. Infact hadoop (from which our annotations are > > derived I believe), talks about this, "Also, certain APIs are annotated > as > > @VisibleForTesting (from com.google.common > .annotations.VisibleForTesting) > > - these are meant to be used strictly for unit tests and should be > treated > > as “Private” APIs." > > > > > > > https://hadoop.apache.org/docs/r3.1.2/hadoop-project-dist/hadoop-common/InterfaceClassification.html > > > > Sorry Bharath, I don't follow. Are you saying "I share the opinion that the > VisibleForTesting annotation should be considered as defining a method as > IA.Private," and this is an omission from our community guidelines > document? Or are you saying "no, it does not count as an interface audience > marker," and we are obliged to treat methods such as in this example as > public API? > > Thanks, > Nick > > On Mon, Jun 22, 2020 at 10:15 AM Sean Busbey wrote: > > > > > Yeah I would say no as well. We should make clear on our dev guide that > > you > > > also should be marking those things with an Interface Audience marking > if > > > you don't intend them to be at the downstream API visibility of the > > parent > > > class. > > > > > > (IIRC we also use VisibleForTesting in IA.Private classes to > proactively > > > explain why some internal looking member is at a wider Java access > > scope.) > > > > > > On Mon, Jun 22, 2020, 11:39 Nick Dimiduk wrote: > > > > > > > Hello, > > > > > > > > This came up over on the 2.3.0RC0 thread, so let's open it for proper > > > > discussion. In that context, we observe method signature changes to a > > > > method marked with the Guava VisibleForTesting annotation. The method > > is > > > a > > > > protected method on a IA.Public class. There is no method-level IA > > > > annotation. > > > > > > > > Do we consider the VisibleForTesting annotation as a specifier for > our > > > > compatibility guidelines? > > > > > > > > I am of the opinion that no, it is not an InterfaceAudience > annotation, > > > and > > > > so it is not applicable for defining our public API. > > > > > > > > What do you think? > > > > > > > > Thanks, > > > > Nick > > > > > > > > > >
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
On Mon, Jun 22, 2020 at 3:45 PM Bharath Vissapragada wrote: > I share the same opinion. Infact hadoop (from which our annotations are > derived I believe), talks about this, "Also, certain APIs are annotated as > @VisibleForTesting (from com.google.common .annotations.VisibleForTesting) > - these are meant to be used strictly for unit tests and should be treated > as “Private” APIs." > > > https://hadoop.apache.org/docs/r3.1.2/hadoop-project-dist/hadoop-common/InterfaceClassification.html > Sorry Bharath, I don't follow. Are you saying "I share the opinion that the VisibleForTesting annotation should be considered as defining a method as IA.Private," and this is an omission from our community guidelines document? Or are you saying "no, it does not count as an interface audience marker," and we are obliged to treat methods such as in this example as public API? Thanks, Nick On Mon, Jun 22, 2020 at 10:15 AM Sean Busbey wrote: > > > Yeah I would say no as well. We should make clear on our dev guide that > you > > also should be marking those things with an Interface Audience marking if > > you don't intend them to be at the downstream API visibility of the > parent > > class. > > > > (IIRC we also use VisibleForTesting in IA.Private classes to proactively > > explain why some internal looking member is at a wider Java access > scope.) > > > > On Mon, Jun 22, 2020, 11:39 Nick Dimiduk wrote: > > > > > Hello, > > > > > > This came up over on the 2.3.0RC0 thread, so let's open it for proper > > > discussion. In that context, we observe method signature changes to a > > > method marked with the Guava VisibleForTesting annotation. The method > is > > a > > > protected method on a IA.Public class. There is no method-level IA > > > annotation. > > > > > > Do we consider the VisibleForTesting annotation as a specifier for our > > > compatibility guidelines? > > > > > > I am of the opinion that no, it is not an InterfaceAudience annotation, > > and > > > so it is not applicable for defining our public API. > > > > > > What do you think? > > > > > > Thanks, > > > Nick > > > > > >
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
I share the same opinion. Infact hadoop (from which our annotations are derived I believe), talks about this, "Also, certain APIs are annotated as @VisibleForTesting (from com.google.common .annotations.VisibleForTesting) - these are meant to be used strictly for unit tests and should be treated as “Private” APIs." https://hadoop.apache.org/docs/r3.1.2/hadoop-project-dist/hadoop-common/InterfaceClassification.html On Mon, Jun 22, 2020 at 10:15 AM Sean Busbey wrote: > Yeah I would say no as well. We should make clear on our dev guide that you > also should be marking those things with an Interface Audience marking if > you don't intend them to be at the downstream API visibility of the parent > class. > > (IIRC we also use VisibleForTesting in IA.Private classes to proactively > explain why some internal looking member is at a wider Java access scope.) > > On Mon, Jun 22, 2020, 11:39 Nick Dimiduk wrote: > > > Hello, > > > > This came up over on the 2.3.0RC0 thread, so let's open it for proper > > discussion. In that context, we observe method signature changes to a > > method marked with the Guava VisibleForTesting annotation. The method is > a > > protected method on a IA.Public class. There is no method-level IA > > annotation. > > > > Do we consider the VisibleForTesting annotation as a specifier for our > > compatibility guidelines? > > > > I am of the opinion that no, it is not an InterfaceAudience annotation, > and > > so it is not applicable for defining our public API. > > > > What do you think? > > > > Thanks, > > Nick > > >
Re: [DISCUSS] VisibleForTesting annotation as it pertains to our API compatibility guidelines
Yeah I would say no as well. We should make clear on our dev guide that you also should be marking those things with an Interface Audience marking if you don't intend them to be at the downstream API visibility of the parent class. (IIRC we also use VisibleForTesting in IA.Private classes to proactively explain why some internal looking member is at a wider Java access scope.) On Mon, Jun 22, 2020, 11:39 Nick Dimiduk wrote: > Hello, > > This came up over on the 2.3.0RC0 thread, so let's open it for proper > discussion. In that context, we observe method signature changes to a > method marked with the Guava VisibleForTesting annotation. The method is a > protected method on a IA.Public class. There is no method-level IA > annotation. > > Do we consider the VisibleForTesting annotation as a specifier for our > compatibility guidelines? > > I am of the opinion that no, it is not an InterfaceAudience annotation, and > so it is not applicable for defining our public API. > > What do you think? > > Thanks, > Nick >
