+1 We shouldn't even reference Apache JIRA tickets. References to
outside things like Trac tickets, JIRA tickets or even commit SHAs
become outdated over time.
On 2/20/19 11:28 AM, Alexander Murmann 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
