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