I didn't write this test, but I did rename it from Bug42226Test to
PersistentPartitionHangsDuringRestartRegressionTest and do some additional
cleaning to bring it up to minimal standards.
Please feel free to file a PR that removes all Jira and Trac ticket #s from
the codebase (or parts of it).
On Wed, Feb 20, 2019 at 11:28 AM Alexander Murmann <amurm...@apache.org>
wrote:
> 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
> > >
> >
>
