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 <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
