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
