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 <ndimi...@apache.org> 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 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
>>
>
