I think it's important that we enable everyone in the community to be
equally successful and on top of that do NOT rely on non-Apache resources.
If we find value in Trac numbers, we should either find a way to make them
accessible to everyone or update the comment so that there is no value in
knowing the number.
In general, I avoid references in the code to anything outside the code
base as much as possible. Anything that's checked into the repo gets
versioned together and I don't have to worry about aligning versions of
what's in the code with potential versions of the outside resource. So my
vote is for augmenting the comment. Maybe we can do even better and make
the code or test self-explanatory enough to need even fewer comments?
On Wed, Feb 20, 2019 at 10:23 AM Kirk Lund <kl...@apache.org> wrote:
> Well, the problem is that different people disagree on what's "meaningful"
> in this context. For example:
>
> See PersistentPartitionHangsDuringRestartRegressionTest.java
>
> * /***
> * * RegressionTest for bug 42226. <br>*
> * * 1. Member A has the bucket <br>*
> * * 2. Member B starts creating the bucket. It tells member A that it
> hosts the bucket <br>*
> * * 3. Member A crashes <br>*
> * * 4. Member B destroys the bucket and throws a partition offline
> exception, because it wasn't*
> * * able to complete initialization. <br>*
> * * 5. Member A recovers, and gets stuck waiting for member B.*
> * **
> * * <p>*
> * * TRAC 42226: recycled VM hangs during re-start while waiting for
> Partition to come online (after*
> * * Controller VM sees unexpected PartitionOffLineException while doing
> ops)*
> * */*
> * @Test*
> * public void doesNotWaitForPreviousInstanceOfOnlineServer() throws
> Exception {*
>
> I personally would NOT remove the references to "42226" from the above, and
> here's why:
> 1) The javadoc clearly states the what and how of this bug
> 2) The 2nd paragraph includes the full summary from the old TRAC ticket
> 3) Quite a few people working on Geode do have access to TRAC which means
> anyone in the community can request us to go dig up further history about
> this bug to share
>
> If we systematically delete all TRAC #s from javadocs and comments then
> community developers will lose that opportunity. While it's true that most
> of the time most developers will probably never need or want that history,
> I would not say it's never going to be valuable.
>
> When I'm working on code that references old bugs (especially regression
> tests like the one above), I frequently pull up TRAC and read about the
> bug. I also tend to pull up the old pre-Apache git repo and read through
> old commit messages. Maybe I'm the only one who does this.
>
> Despite my point of view, I don't feel very strongly about it. If you guys
> really think that the TRAC #s distract or confuse you too much and you
> can't foresee the need to ask someone to pull up history on a ticket, then
> you should submit a PR to "remove all TRAC #s" from the code base. I won't
> disapprove it -- I can always look at older revisions of files like
> PersistentPartitionHangsDuringRestartRegressionTest to find the old TRAC #.
> I previously submitted PRs to rename all test classes and test methods from
> names like test42226 to meaningful names so I support the overall intent.
>
> On Tue, Feb 19, 2019 at 5:21 PM Jacob Barrett <jbarr...@pivotal.io> wrote:
>
> > Comments that don’t provide meaningful context beyond what is already
> > expressed in the code should be removed. A number to a system that the
> > general public can’t access is not meaningful. Delete or replace with
> > meaningful comment.
> >
> > -jake
> >
> >
> > > On Feb 19, 2019, at 1:41 PM, Michael Oleske <mole...@pivotal.io>
> wrote:
> > >
> > > Hey Geode Dev Friends!
> > >
> > > I was reviewing a PR (this one
> https://github.com/apache/geode/pull/3197
> > )
> > > and made a note that maybe we should remove comments that make
> references
> > > to bug and trac numbers that people cannot reach (like me for one).
> Kirk
> > > mentioned that some people (like him) have access to those bugs and
> have
> > > proven helpful for some history. So there is this balance between
> noise
> > > (people who cannot access those old issues) and getting context (people
> > who
> > > can access those issues).
> > >
> > > So I guess my point is to start a discussion on what a path forward
> might
> > > be (if any)? My opinion is that they are noise and we should remove
> > them.
> > > If someone has access to the original issue, then making sure there is
> a
> > > test case covering it should be done. Then it makes even more sense to
> > me
> > > to remove the comment.
> > >
> > > -michael
> >
>
