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

Reply via email to