I'm not sure how a downstream developer is supposed to know what is and is not part of our public API through casual inspection. For example, this method has no indication that the VisibleForTesting is present via the generated javadocs,
https://hbase.apache.org/2.2/devapidocs/org/apache/hadoop/hbase/tool/LoadIncrementalHFiles.html#tryAtomicRegionLoad-org.apache.hadoop.hbase.client.ClientServiceCallable-org.apache.hadoop.hbase.TableName-byte:A-java.util.Collection- On Wed, Jun 24, 2020 at 2:09 PM Nick Dimiduk <ndimi...@apache.org> wrote: > On Wed, Jun 24, 2020 at 14:07 Andrew Purtell <apurt...@apache.org> wrote: > >> I am -1 on treating VisibleForTesting as public API. Semantically it makes >> no sense. > > > By extension then you think it’s acceptable to define our public api in > terms of this annotation provided by Guava? I think that 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 >> >
