Thanks Phil and Tim, they're good points. >From the above, I can gather that although there is some extra burden on authors, we can keep it the way it is, and add onto Phil's Jenkins email notification change to explicitly notify authors when cherry-picks to 2.x fail, since failures going unnoticed is 80% of the problem.
A review bot warning is also a good idea (but not necessarily a must have), especially for when people forget to add "not for 2.x". A default comment of "Are you sure this should go into 2.x" would help. I don't think we're at a point where a majority of commits shouldn't go into '2.x' from 'master'. We can switch to explicit cherry-picking when that time comes. Of course if people feel differently, they should speak up. - Sailesh On Thu, May 3, 2018 at 12:53 PM, Philip Zeyliger <phi...@cloudera.com> wrote: > I don't like writing "Cherry-picks: not for 2.x" in commit messages and > then subsequently doing the cherry-pick. That's sending a confusing message > in the commit message. We can re-word the sigils if we want to say > something about "don't cherry-pick automatically but I'll do it myself" but > that just seems to delay the inevitable, and, all things equal, I'd rather > the ordering be consistent. > > Is the problem the "unnoticed" part of your original e-mail? I'm starting > to make it possible for jenkins.impala.io to send e-mails, so it'll be > possible for it to send e-mails to the committer of the first broken > change, nudging them along. Would that help? > > Should we stop automatically cherry-picking everything? Now that 3.0 and > 2.12 are (nearly/already) released, should we switch to explicit > cherry-picking? > > I'd also be quite happy to start experimenting with review bots that > comment on things like "is this a clean cherry-pick" on reviews > automatically. > > -- Philip > > > On Thu, May 3, 2018 at 12:35 PM, Tim Armstrong <tarmstr...@cloudera.com> > wrote: > > > Maybe we should just start with posting back a warning to the code review > > if the change doesn't cleanly apply? > > > > On Thu, May 3, 2018 at 12:34 PM, Tim Armstrong <tarmstr...@cloudera.com> > > wrote: > > > > > It would be good to encourage people to proactively do cherry-picks > but I > > > think it needs a bit more thought before automating it or adding more > > > barriers. > > > > > > That could potentially cause hold-ups if there's a dependency on a > > > previous patch. E.g. if I'm merging two patches B and C that depend on > > each > > > other and B gets merged, then the state while B is being tested is: > > > > > > Master > > > B->A->... > > > > > > 2.x > > > A->... > > > > > > and B is in the process of being tested, then what you really want is > for > > > a new patch to cleanly apply on top of B, but I think your proposed > check > > > is that it applies on top of A. > > > > > > It's also introduces the possibility of more mistakes if we encourage > > > people to do the cherry-pick to 2.x before precommit tests for master > > have > > > passed, since they may be cherry-picking a version that doesn't pass > > those > > > tests. People can of course repeat the cherry-picking process each time > > > they push out a new revision but it does introduce more risk of > > > accidentally diverging your master and 2.x commits versus just doing > the > > > conflict resolution once. > > > > > > On Thu, May 3, 2018 at 12:17 PM, Lars Volker <l...@cloudera.com> wrote: > > > > > >> Should we add a check to the master GVO that makes sure that a change > > >> either applies to 2.x, or has a "not for 2.x" label? > > >> > > >> On Thu, May 3, 2018 at 12:12 PM, Sailesh Mukil <sail...@cloudera.com> > > >> wrote: > > >> > > >> > It's been a while since we started maintaining 2 active branches, > > master > > >> > and 2.x, and the divergence between them has grown quite a bit. > > >> > > > >> > The implications of this (as Michael Brown brought to my attention) > > are > > >> > that a patch that goes into 'master' may not cleanly apply to '2.x', > > >> which > > >> > would fail the cherrypick-2.x-and-test( > > >> > https://jenkins.impala.io/job/cherrypick-2.x-and-test/) job, and if > > >> > unnoticed, can hold up all future cherry-picks from 'master' to > '2.x' > > >> until > > >> > this is resolved. > > >> > > > >> > If this does happen, the original author of the patch needs to > > manually > > >> > cherry-pick the patch from 'master' to '2.x', resolve the conflicts > > and > > >> > push a change for review to the '2.x' branch with the same > Change-Id. > > >> > > > >> > To avoid these types of failures, I advise authors to locally cherry > > >> pick > > >> > their patch to '2.x' to see if it applies cleanly before running > GVO. > > >> > > > >> > Lastly, if a patch is meant only for 'master', and not for '2.x', > > please > > >> > remember to add the "Cherry-picks: not for 2.x" line to your commit > > >> > message. There have been a few patches in which this was not > included, > > >> > which unfortunately requires manual work to resolve later on. > > >> > > > >> > Thank you for your time! > > >> > > > >> > - Sailesh > > >> > > > >> > > > > > > > > >
