As I suggested before, replace them with Javadoc.
On Wed, Jun 24, 2020 at 2:10 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 > > > -- Best regards, Andrew Words like orphans lost among the crosstalk, meaning torn from truth's decrepit hands - A23, Crosstalk
