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 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 <bus...@apache.org> 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 <apurt...@apache.org>
> > 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>
> > > > > 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 <bus...@apache.org> 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
> > > 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 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 <
> > > ndimi...@apache.org>
> > > > > > > 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 <
> wellington.chevre...@gmail.com
> > >
> > > > > > > > > 于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."
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> 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
> <
> > > > > > > > > > bus...@apache.org>
> > > > > > > > > > > > > > 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
> <
> > > > > > > > > > > ndimi...@apache.org>
> > > > > > > > > > > > > > > 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
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > Best regards,
> > > > > > > > > Andrew
> > > > > > > > >
> > > > > > > > > Words like orphans lost among the crosstalk, meaning torn
> > from
> > > > > > truth's
> > > > > > > > > decrepit hands
> > > > > > > > > - A23, Crosstalk
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Best regards,
> > > > > Andrew
> > > > >
> > > > > Words like orphans lost among the crosstalk, meaning torn from
> > truth's
> > > > > decrepit hands
> > > > > - A23, Crosstalk
> > > > >
> > > >
> > >
> > >
> > > --
> > > Best regards,
> > > Andrew
> > >
> > > Words like orphans lost among the crosstalk, meaning torn from truth's
> > > decrepit hands
> > > - A23, Crosstalk
> > >
> >
>
>
> --
> Best regards,
> Andrew
>
> Words like orphans lost among the crosstalk, meaning torn from truth's
> decrepit hands
> - A23, Crosstalk
>
