Hi David On Fri, Jan 27, 2023 at 10:11 PM David Capwell <dcapw...@apple.com> wrote:
> I've learned that when I have defended the need (or right, if appealing to > the Governance texts...) for contributors to be able to review a feature > branch at the time it is merged to trunk - which for Accord is now - that a > common reaction to this is that doing a review of Accord now might take > months and would stall the Accord project for months if that is allowed. > > > The way I have been reading this thread is not that “we don’t want the > review as it slows us down” but more “the process is X, why are you asking > for Y?”. I believe I can speak for everyone working on Accord, we all want > more reviewers and contributors! > > I know the Accord team has all the time been open to and even invited participation from other contributors. I believe what happened was simply that some of us thought "the process is Y" and then were surprised to be rejected when trying to do Y. I think this thread has already surfaced the different assumptions and what might be the most meaningful consensus for future similar projects. > I think its fair to ask the question on if feature branches “should" have > the same process as non-feature branches, but since that has not been > called out and voted on they are expected to follow the same process; if > people working on feature branches have not been following the same process > that is currently an issue that needs to be addressed (In the case of > Accord all contributors are Committers or PMC and same process has been > followed as trunk). The project doesn’t have a good history (at least > recent history) of open development of features, most features are dumps > from private sources, so there are learning/growing pains as we try to > develop new features in the open. > > Yup. > On the other hand, I can think of many things that a pair of fresh eyes > can do at this point in reasonable time, like days or a couple weeks. > > > I agree more eyes are better, but the conversation is on code that has > already merged. I know that many of us review code after it has been > merged, and what we have been doing is filing follow up bugs/improvement > requests as new work. A simple example of this was when the trie memtables > patch landed I reviewed code that was merged I think 2021 making memtables > pluggable; I found a bug in it, showed the bug existed, and worked with the > authors to get all this addressed. Reviews after the fact are fine, > common, and welcome in this project, but that has always sponsored new > tickets and new work to address the feedback. > > I believe Ekaterina was the first one to talk to me about the possibility of reviewing post merge. I have to admit I have simply not taking it seriously at the time. My background is perhaps in more corporate open source development, and I have plenty of experience with situations where even if an engineer, including myself sometimes, wanted to work on fixing some technical debt, the employer's project management processes simply wouldn't prioritize that work and it was left for years. Now that several people have assured me that this is actually a thing in the Cassandra project, I will try to embrace that option. It's certainly a nice tool to not hold up the merge more than necessary. On Fri, Jan 27, 2023 at 10:22 PM Josh McKenzie <jmcken...@apache.org> wrote: > I know that many of us review code after it has been merged, and what we > have been doing is filing follow up bugs/improvement requests as new work. > > I think this is key. The code on the feature branch matches the same bar > of quality / process that's on trunk, and it's trivially easy to checkout > at the merge SHA and review the code in an IDE even after merge, raising > questions with someone if you have concerns. If we were talking about > merging this feature branch a week before a GA I could see why there'd be > concern but we have a lot of calendar runway here. > > Ah, this is a great perspective! I can immediately think of follow-up thoughts... 1. Don't we follow a principle of always shippable trunk? This was actually a reason why I sidelined the talk about post-merge review, because it implies that the code wasn't "good enough"/perfect when it was first merged. (But life is of course never that black and white.) Anyway... With always shippable trunk, in theory it shouldn't make a difference whether this is happening a week before GA or months before. But, more importantly... 2. You and others are saying the Accord feature branch followed the same principles as development on trunk. Does this mean there have also been nightly jenkins builds running? Is there a history of such test results visible somewhere? If yes, I think that lends a lot of credibility to the claim the process was as rigorous as it is for trunk, and looking at the build history for a few minutes should put our minds at ease. I can't see anything Accord related in Jenkins or Butler? But perhaps there's a CircleCI dashboard somewhere? >From all the discussions I've had the past week, it seems the emerging consensus is that protecting the stability of the CI pipeline is that top concern. Other topics discussed (like comments in the code) are smaller issues relatively. If the feature branch indeed has all the CI machinery setup as if it was trunk, then I agree chances are good it will merge into trunk smoothly. If the CI coverage isn't 100%, then we should just identify the gaps, and focus on that while preparing to merge. Either way, I know everyone is at this point committed to CI stability, including the Accord authors, so I'm not particularly worried personally. > 1. Code must not be committed if a committer has requested reasonable > time to conduct a review > > I'm realizing in retrospect this leaves ambiguity around *where* the code > is committed (i.e. trunk vs. feature branch). On the one hand we could say > this code has already been reviewed and committed; the feature branch had > the same bar as trunk. On the other hand you could say the intent of this > agreement was to allow committers to inspect code *before it goes to > trunk*, which would present a different set of constraints. > > Without further qualification, I would have expected the latter, but I realize here are good examples to support the opposite interpretation too. I actually don't know that we need to rush to clarify that either. This is the first time the situation came up, and after this discussion I'm sure things will be clearer. > My perspective is that of the former; I consider this code "already > committed". This work was done on a formal feature branch w/JIRA tickets > and feedback / review done on the PR's in the open on the feature branch by > 2+ committers, so folks have had a reasonable time to engage with the > process and conduct a review. I don't think right on the cusp of a > ceremonial / procedural cutover from a feature branch to trunk is the right > time, nor scope of work, to propose a blocking review based on this text. > > Call me conservative but I still don't think a dump / merge / addition of 28k + 49k lines of code (over 10% of the codebase!) is ceremonial / procedural at all. The big CEPs I've been involved with in these past two years (SAI, Tries, UCS...) are about 10-20k each I think, and I thought those are big, and many of us worry about the impact to CI for those as well. (For example, if this branch is mostly tested in CircleCi, and not heavily on the ASF Jenkins, it could fail even for trivial reasons, like running out of disk or RAM...) I hope we see this merged to trunk very smoothly, but that's smoothly like a brigg, not smoothly as a laser class boat. henrik PS: My current status is that I've learned a lot about accord the past week, even stumbled into a block of code that presumably should be uncommented (now or later)... But the discussion on remaining items should have been delegated to the more relevant contributors than myself. If you don't see further communications from me, it just means I'm satisfied for my part and expect others to speak up (or not) for those parts. (I guess I will quietly continue to keep an eye on the CI aspect...) -- Henrik Ingo c. +358 40 569 7354 w. www.datastax.com <https://www.facebook.com/datastax> <https://twitter.com/datastax> <https://www.linkedin.com/company/datastax/> <https://github.com/datastax/>
