We could commit first and then GVO -- that way if a commit broke something (in a reproducible way), we would at least know which commit did it. In this proposal, we don't have to wait for GVO N to finish before committing patch N+1. This is a sort of fast-path-slow-path idea, but fixing the slow path while keeping 2.x stable could be tricky.
On Fri, Jan 19, 2018 at 3:53 PM, Dimitris Tsirogiannis <[email protected]> wrote: > My ideal and more conservative policy would be: > - If conflicts, review. > - Always GVO. > > Dimitris > > On Fri, Jan 19, 2018 at 2:13 PM, Taras Bobrovytsky <[email protected]> > wrote: > >> I like your policy suggestion. If there are no conflicts, then we can push >> from master to 2.x without any tests or reviews. If there are conflicts, >> then a review AND a GVO test run are required. >> >> We can at least start off with this policy and then change it if it is not >> working well for some reason. >> >> On Fri, Jan 19, 2018 at 1:59 PM, Philip Zeyliger <[email protected]> >> wrote: >> >> > An update here! >> > >> > I'm pretty close to pushing the first master-only change (the very >> exciting >> > 1-liner at https://gerrit.cloudera.org/#/c/9044/ that bumps versions). >> > After that, I'll be cherrypicking things into 2.x. >> > >> > We need a policy, I think, on reviewing these cherry-picks. The most >> > heavy-weight would be to do Gerrit and run-tests-before-merge for every >> > change. The least heavy would be to review only when the cherry-picks >> > aren't clean. Are people comfortable with the less heavy policy? >> > >> > Specifically, I propose that clean cherrypicks from master to 2.x can be >> > pushed without additional review. >> > >> > Thanks! >> > >> > -- Philip >> > >> > On Wed, Jan 17, 2018 at 9:57 AM, Philip Zeyliger <[email protected]> >> > wrote: >> > >> > > It sounds like we've reached consensus, so I'm starting to maneuver >> this >> > > along. Please don't hesitate if you've got concerns. You can follow >> along >> > > at https://issues.apache.org/jira/browse/IMPALA-6410. >> > > >> > > The first two reviews are out at: >> > > >> > > remote: http://gerrit.cloudera.org:8080/9044 Bumping version to 3.0. >> > > remote: http://gerrit.cloudera.org:8080/9045 IMPALA-6410: Tool to >> > > cherrypick changes across branches. >> > > >> > > I've created the branches on Gerrit and Apache as follows: >> > > >> > > # This created the branch on gerrit.cloudera.org >> > > $ssh -p 29418 [email protected] gerrit create-branch >> Impala-ASF >> > > 2.x master >> > > >> > > # This created the branch on the Apache git server: >> > > $git push apache asf-gerrit/2.x:refs/heads/2.x >> > > Username for 'https://git-wip-us.apache.org': philz >> > > Password for 'https://[email protected]': >> > > Total 0 (delta 0), reused 0 (delta 0) >> > > To https://git-wip-us.apache.org/repos/asf/impala.git >> > > * [new branch] asf-gerrit/2.x -> 2.x >> > > >> > > # Double-check: >> > > $git-ls-remote apache | grep 2.x; git-ls-remote asf-gerrit | grep 2.x >> > > 6cc76d72016b8d5672676e8e8a979b0807803ad9 refs/heads/2.x >> > > 6cc76d72016b8d5672676e8e8a979b0807803ad9 refs/heads/2.x >> > > >> > > # push_to_asf learned of the branch "magically" >> > > $bin/push_to_asf.py >> > > INFO:root:Fetching from remote 'asf-gerrit'... >> > > INFO:root:done >> > > Branch '2.x': up to date >> > > Branch 'asf-site': up to date >> > > Branch 'branch-2.10.0': found on Apache but not in gerrit >> > > Branch 'branch-2.11.0': found on Apache but not in gerrit >> > > Branch 'branch-2.7.0': found on Apache but not in gerrit >> > > Branch 'branch-2.8.0': found on Apache but not in gerrit >> > > Branch 'branch-2.9.0': found on Apache but not in gerrit >> > > Branch 'hadoop-next': up to date >> > > Branch 'master': up to date >> > > >> > > -- Philip >> > > >> > > On Tue, Jan 16, 2018 at 5:29 PM, Alexander Behm < >> [email protected]> >> > > wrote: >> > > >> > >> +1 >> > >> >> > >> On Tue, Jan 16, 2018 at 5:27 PM, Taras Bobrovytsky < >> [email protected] >> > > >> > >> wrote: >> > >> >> > >> > +1 >> > >> > >> > >> > On Tue, Jan 16, 2018 at 1:34 PM, Lars Volker <[email protected]> >> wrote: >> > >> > >> > >> > > +1 >> > >> > > >> > >> > > On Tue, Jan 16, 2018 at 1:29 PM, Philip Zeyliger < >> > [email protected] >> > >> > >> > >> > > wrote: >> > >> > > >> > >> > > > Hi folks! >> > >> > > > >> > >> > > > It sounds like there haven't been objections to having master be >> > >> "3.0" >> > >> > > and >> > >> > > > introducing a 2.x branch. Would folks be alright if I started >> > making >> > >> > > > changes in that direction? >> > >> > > > >> > >> > > > Thanks! >> > >> > > > >> > >> > > > -- Philip >> > >> > > > >> > >> > > > On Fri, Jan 12, 2018 at 4:10 PM, Philip Zeyliger < >> > >> [email protected]> >> > >> > > > wrote: >> > >> > > > >> > >> > > > > >> > >> > > > > >> > >> > > > > On Fri, Jan 12, 2018 at 4:04 PM, Jim Apple < >> > [email protected]> >> > >> > > wrote: >> > >> > > > > >> > >> > > > >> This makes sense to me. >> > >> > > > >> >> > >> > > > >> In this mode, for 2.x-only changes and for changes on 3.0 >> that >> > >> don't >> > >> > > > >> apply cleanly, there will be a manual way to do the step >> > labelled >> > >> > "1. >> > >> > > > >> Cherrypick tool", and that way is the same way we send >> patches >> > >> for >> > >> > > > >> review now, but pushing to HEAD:refs/for/2.x rather than >> > >> > > > >> HEAD:refs/for/master, yes? >> > >> > > > >> >> > >> > > > > >> > >> > > > > Exactly. So, non-clean cherrypicks or 2.x-only changes go >> > through >> > >> > > review >> > >> > > > > on Gerrit, but we give an implicit review pass to clean >> > >> cherrypicks. >> > >> > > > > >> > >> > > > > We could have the cherrypick tool between gerrit/master and >> > >> > gerrit/2.x >> > >> > > do >> > >> > > > > the cherrypicks and run the tests on Jenkins. Do you think >> > that's >> > >> > > > > preferable? >> > >> > > > > >> > >> > > > > -- Philip >> > >> > > > > >> > >> > > > > >> > >> > > > > >> > >> > > > >> >> > >> > > > >> On Fri, Jan 12, 2018 at 3:57 PM, Philip Zeyliger < >> > >> > [email protected] >> > >> > > > >> > >> > > > >> wrote: >> > >> > > > >> > Picture: >> > >> > > > >> > https://gist.github.com/philz/ >> 323c8b4cb411dc12eb7231d922c195 >> > >> > > > >> 1f#file-impala-branch-image-pdf >> > >> > > > >> > >> > >> > > > >> > >> > >> > > > >> > On Fri, Jan 12, 2018 at 3:47 PM, Jim Apple < >> > >> [email protected]> >> > >> > > > >> wrote: >> > >> > > > >> > >> > >> > > > >> >> Often, this list seems to filter out images. Could you >> post >> > it >> > >> > and >> > >> > > > >> send a >> > >> > > > >> >> link? >> > >> > > > >> >> >> > >> > > > >> >> Thanks for taking this on, Phil! >> > >> > > > >> >> >> > >> > > > >> >> On Fri, Jan 12, 2018 at 3:15 PM, Philip Zeyliger < >> > >> > > > [email protected]> >> > >> > > > >> >> wrote: >> > >> > > > >> >> >> > >> > > > >> >> > I think most patches go to Gerrit branch 'master', which >> > >> > happens >> > >> > > to >> > >> > > > >> >> > identify itself as 3.0. (Or 3.x?). >> > >> > > > >> >> > >> > >> > > > >> >> > Here's a picture: >> > >> > > > >> >> > >> > >> > > > >> >> > [image: Inline image 1] >> > >> > > > >> >> > >> > >> > > > >> >> > >> > >> > > > >> >> > With this, every time "cherrypick_and_push_to_asf.py" >> is >> > >> run, >> > >> > it >> > >> > > > >> would >> > >> > > > >> >> > first offer to cherrypick changes between master and >> 2.x. >> > >> Then, >> > >> > > it >> > >> > > > >> would >> > >> > > > >> >> > offer push those cherrypicks to gerrit/2.x. After that, >> it >> > >> > > > continues >> > >> > > > >> on >> > >> > > > >> >> as >> > >> > > > >> >> > before and offers to push changes to ASF. I think this >> > >> > maintains >> > >> > > > the >> > >> > > > >> >> > invariant that pushing to ASF is only done with a human >> > >> > trigger. >> > >> > > > (We >> > >> > > > >> >> could >> > >> > > > >> >> > also have step 1 be done by a Jenkins robot, since it's >> > >> between >> > >> > > > >> Gerrit >> > >> > > > >> >> and >> > >> > > > >> >> > Gerrit.) >> > >> > > > >> >> > >> > >> > > > >> >> > I looked at the How to Release page, and the main >> > difference >> > >> > > would >> > >> > > > be >> > >> > > > >> >> > that, for a 2.x release, the $COMMIT_HASH_YOU_CHOSE >> would >> > >> come >> > >> > > from >> > >> > > > >> the >> > >> > > > >> >> 2.x >> > >> > > > >> >> > branch, as would any cherrypicks. >> > >> > > > >> >> > >> > >> > > > >> >> > Does this match what you're thinking? >> > >> > > > >> >> > >> > >> > > > >> >> > Thanks! >> > >> > > > >> >> > >> > >> > > > >> >> > -- Philip >> > >> > > > >> >> > >> > >> > > > >> >> > On Fri, Jan 12, 2018 at 11:07 AM, Jim Apple < >> > >> > > [email protected]> >> > >> > > > >> >> wrote: >> > >> > > > >> >> > >> > >> > > > >> >> >> Which gerrit branch were you thinking most patches >> would >> > go >> > >> > to? >> > >> > > > >> >> >> >> > >> > > > >> >> >> If they go to 3.0, then push_to_asf.py would have to be >> > >> > amended >> > >> > > to >> > >> > > > >> >> >> push to gerrit, bypassing code review. I think that's >> > >> > possible, >> > >> > > > but >> > >> > > > >> >> >> I'm not 100%. >> > >> > > > >> >> >> >> > >> > > > >> >> >> There is also security to think about, since the >> > >> > push_to_asf.py >> > >> > > > >> users >> > >> > > > >> >> >> can push a few commits at a time, including ones they >> > >> didn't >> > >> > > > author >> > >> > > > >> or >> > >> > > > >> >> >> review. >> > >> > > > >> >> >> >> > >> > > > >> >> >> We'll also want to clarify >> > >> > > > >> >> >> https://cwiki.apache.org/ >> confluence/display/IMPALA/How+ >> > >> > > to+Release >> > >> > > > >> and >> > >> > > > >> >> >> keep it consistent with the git & gerrit statuses quo. >> > >> > > > >> >> >> >> > >> > > > >> >> >> On Fri, Jan 12, 2018 at 10:52 AM, Philip Zeyliger < >> > >> > > > >> [email protected]> >> > >> > > > >> >> >> wrote: >> > >> > > > >> >> >> > Hi! >> > >> > > > >> >> >> > >> > >> > > > >> >> >> >> Should we start tagging all candidates with a common >> > >> label, >> > >> > > > e.g. >> > >> > > > >> >> >> > include-in-v3? >> > >> > > > >> >> >> > >> > >> > > > >> >> >> > I agree with Lars's suggestion for tagging JIRAs with >> > >> > > > >> include-in-v3. >> > >> > > > >> >> >> I've >> > >> > > > >> >> >> > done so, and the relevant query is >> > >> > > > >> >> >> > https://issues.apache.org/ >> > jira/issues/?jql=labels%20%3D% >> > >> > 20in >> > >> > > > >> >> >> clude-in-v3%20and%20project%3Dimpala >> > >> > > > >> >> >> > . >> > >> > > > >> >> >> > >> > >> > > > >> >> >> >> What sort of process were you thinking of for the >> > >> > automation? >> > >> > > > >> >> >> > >> > >> > > > >> >> >> > I think amending push_to_asf.py, as you suggest, is a >> > >> great >> > >> > > > idea. >> > >> > > > >> I >> > >> > > > >> >> >> think >> > >> > > > >> >> >> > we have a string ("not for 2.x") which can be used in >> > >> commit >> > >> > > > >> messages >> > >> > > > >> >> to >> > >> > > > >> >> >> > discourage the cherrypick for the changes we want to >> be >> > >> > > > exclusive >> > >> > > > >> >> until >> > >> > > > >> >> >> we >> > >> > > > >> >> >> > want to change the defaults in the other direction. >> > >> (I.e., >> > >> > > right >> > >> > > > >> now >> > >> > > > >> >> the >> > >> > > > >> >> >> > string is "not for 2.x", but at some point the string >> > >> may be >> > >> > > > >> "should >> > >> > > > >> >> be >> > >> > > > >> >> >> > cherrypicked to 2.x".) >> > >> > > > >> >> >> > >> > >> > > > >> >> >> > I do think that we want to create a gerrit branch to >> > >> allow >> > >> > > > >> 2.x-only >> > >> > > > >> >> >> changes >> > >> > > > >> >> >> > to be reviewed in the straight-forward fashion. >> > >> > > > >> >> >> > >> > >> > > > >> >> >> > -- Philip >> > >> > > > >> >> >> > >> > >> > > > >> >> >> > >> > >> > > > >> >> >> > >> > >> > > > >> >> >> > On Thu, Jan 11, 2018 at 9:31 AM, Jim Apple < >> > >> > > > [email protected]> >> > >> > > > >> >> >> wrote: >> > >> > > > >> >> >> > >> > >> > > > >> >> >> >> I'm on-board with all of this. (I also would be OK >> > >> delaying >> > >> > > > 3.0, >> > >> > > > >> if >> > >> > > > >> >> >> >> that were the consensus). >> > >> > > > >> >> >> >> >> > >> > > > >> >> >> >> There is one issue in here I think we should dive >> > into: >> > >> > > > >> >> >> >> >> > >> > > > >> >> >> >> > Both master and 2.x would be active, and, at least >> > for >> > >> > the >> > >> > > > >> >> beginning, >> > >> > > > >> >> >> >> > changes would automatically be pulled into the 2.x >> > >> line, >> > >> > > > unless >> > >> > > > >> >> >> >> explicitly >> > >> > > > >> >> >> >> > blacklisted. >> > >> > > > >> >> >> >> >> > >> > > > >> >> >> >> What sort of process were you thinking of for the >> > >> > automation? >> > >> > > > >> >> >> >> >> > >> > > > >> >> >> >> Some context, starting from what we all likely >> already >> > >> > know: >> > >> > > > >> >> >> >> >> > >> > > > >> >> >> >> The bulk of the code review and pre-merge testing >> > >> results >> > >> > are >> > >> > > > >> >> recorded >> > >> > > > >> >> >> >> in gerrit. Once the pre-merge testing passes, a >> patch >> > is >> > >> > > > >> >> cherry-picked >> > >> > > > >> >> >> >> to the git repo hosted with gerrit. To get the patch >> > to >> > >> the >> > >> > > > >> Impala >> > >> > > > >> >> git >> > >> > > > >> >> >> >> repo hosted by the ASF, bin/push_to_asf.py is run >> by a >> > >> > human >> > >> > > > who >> > >> > > > >> >> >> >> supplies his or her ASF credentials. That script >> > copies >> > >> the >> > >> > > > >> commit to >> > >> > > > >> >> >> >> the ASF git repo. >> > >> > > > >> >> >> >> >> > >> > > > >> >> >> >> Often, 2-3 commits will pile up in gerrit before >> some >> > >> > > committer >> > >> > > > >> runs >> > >> > > > >> >> >> >> that script and pushes them to ASF. >> > >> > > > >> >> >> >> >> > >> > > > >> >> >> >> We could edit that script (bin/push_to_asf.py) to >> help >> > >> with >> > >> > > the >> > >> > > > >> >> cherry >> > >> > > > >> >> >> >> picks, so that each time a commit is made, the >> > committer >> > >> > must >> > >> > > > say >> > >> > > > >> >> >> >> whether the commit goes in 2.x, 3.0, or both, but >> the >> > >> > commits >> > >> > > > are >> > >> > > > >> >> >> >> often made by people who didn't author the patches, >> so >> > >> they >> > >> > > may >> > >> > > > >> not >> > >> > > > >> >> be >> > >> > > > >> >> >> >> sure which branch to go in. >> > >> > > > >> >> >> >> >> > >> > > > >> >> >> >> Additionally, gerrit code review is intimately tied >> to >> > >> the >> > >> > > git >> > >> > > > >> repo. >> > >> > > > >> >> >> >> Gerrit runs a git repo under-the-hood, and I believe >> > >> that >> > >> > the >> > >> > > > >> branch >> > >> > > > >> >> >> >> on gerrit's git that changes are cherry-picked to >> > after >> > >> > > > pre-merge >> > >> > > > >> >> >> >> testing is identical to the Impala git repo hosted >> by >> > >> the >> > >> > > ASF - >> > >> > > > >> down >> > >> > > > >> >> >> >> to the hashes, even. If we think 2.x and 3.0 will >> > >> diverge >> > >> > > > enough >> > >> > > > >> that >> > >> > > > >> >> >> >> we'll want different code reviews for different >> > >> branches, >> > >> > > then >> > >> > > > we >> > >> > > > >> >> >> >> might want two different branches on gerrit, too. >> > >> > > > >> >> >> >> >> > >> > > > >> >> >> >> > >> > > > >> >> > >> > >> > > > >> >> > >> > >> > > > >> >> >> > >> > > > >> >> > >> > > > > >> > >> > > > > >> > >> > > > >> > >> > > >> > >> > >> > >> >> > > >> > > >> > >>
