Re: Process for reviewing submitted patches?
I typically refer to the HTC on questions like this, it currently says "We are currently discussing on the list how to adapt our workflow.". Perhaps it's just a matter or someone cleaning up the doc? https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute Also I noticed Dan's JIRA has a patch attached.. and I can't get to JIRA at the moment (jira is down again) but gh lists the PR being created 7 days ago. What happens to folks that submitted a patch prior to the cutover, they are in limbo. etc Jira used to allow us to prioritize "patch availables", given our limited resources. gh is just a big long list which makes it difficult to do similar. Patrick On Wed, Aug 16, 2017 at 12:54 PM, Jordan Zimmerman < jor...@jordanzimmerman.com> wrote: > I thought we've moved to Pull Requests on Github. I've stopped posting > patches. > > -JZ > > > On Aug 16, 2017, at 7:15 PM, Patrick Huntwrote: > > > > On Wed, Aug 16, 2017 at 9:51 AM, Jordan Zimmerman < > > jor...@jordanzimmerman.com> wrote: > > > >> * Review other people's patch. If you help out, others will be more > willing > >> to do the same for you. If someone is kind enough to review your code, > you > >> should return the favor to for someone else. > >> > >> > >> That's fair - I should personally try to do more of this. I'll make an > >> effort here. > >> > >> > > It's not clear to me how we are identifying patches for review today. We > > used to have a very clear process - a jira needed to be in the "patch > > available" state in order to be considered for commit. > > > > See "contribute" section here, notice that it's watered down from what it > > used to be: > > https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute > > > > Dan's patch is not in "patch available" state, is that one of the reasons > > why it's not being moved forward? > > > > Patrick > > > > > >> -Jordan > >> > >
Re: Process for reviewing submitted patches?
I thought we've moved to Pull Requests on Github. I've stopped posting patches. -JZ > On Aug 16, 2017, at 7:15 PM, Patrick Huntwrote: > > On Wed, Aug 16, 2017 at 9:51 AM, Jordan Zimmerman < > jor...@jordanzimmerman.com> wrote: > >> * Review other people's patch. If you help out, others will be more willing >> to do the same for you. If someone is kind enough to review your code, you >> should return the favor to for someone else. >> >> >> That's fair - I should personally try to do more of this. I'll make an >> effort here. >> >> > It's not clear to me how we are identifying patches for review today. We > used to have a very clear process - a jira needed to be in the "patch > available" state in order to be considered for commit. > > See "contribute" section here, notice that it's watered down from what it > used to be: > https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute > > Dan's patch is not in "patch available" state, is that one of the reasons > why it's not being moved forward? > > Patrick > > >> -Jordan >> smime.p7s Description: S/MIME cryptographic signature
Re: Process for reviewing submitted patches?
A few thoughts: 1) It is impossible for us to set SLAs for ZK patches to be reviewed. If we were a company making money on ZK and guaranteeing support for customers who paid us, perhaps we could do that (and for all I know, it's possible that customers with contracts at various companies that rely on ZK for their products do get this). I've been watching this project for many years now, and because there is no one company that is "The ZooKeeper Company", it's always a hit-or-miss level of participation. 2) Part of the reason it's a hit-or-miss project is that it is, for better or worse, somewhat complex, and very mission-critical. Especially when we get new features, determining whether these features make sense for the operational boundaries of the system is non-trivial. I don't think the community wants us to rush in patches to just see the project change (although if you do, please let's hear it). 3) If you want to get your patches committed, you should expect to follow-up with the group until it happens. This is a community where polite reminders can be effectively used to cause movement. Again, see: many of us are truly volunteers. It is also helpful if you make sure that your patches have tests if at all possible, and generally follow the coding standards. If you commit something and you get a -1 from reviewbot, actually addressing that -1 will help. Explaining what you're doing and why helps a lot. Many of you do this, but it's certainly not something we always see in every patch. We're doing better now than we have been in the past, largely thanks to a lot of attention recently from a subset of the committers (not including me sorry, I'm writing this email from my vacation which is about the only time I ever have to focus on the project). Michael had some great comments on how the community can help, so follow his lead. Thanks, C On Wed, Aug 16, 2017 at 1:26 PM, Michael Hanwrote: > We are using github pull request instead of the old patch approach since > last October. So the status of JIRA is irrelevant now (in particular, Patch > Available will not trigger Jenkins pre-commit workflow now.). This was > discussed on dev list when we moved to github, the thread's name is "[VOTE] > move Apache Zookeeper to git". > > As for how to identify available patches for review, they should be all > here: > https://github.com/apache/zookeeper/pulls > > To get notified for new incoming pull requests: > * Watch our github repo: https://github.com/apache/zookeeper > Or > * Subscribe to dev mailing list. Because we have git -> JIRA hook any new > pull request will get cross posted to JIRA which will then be forwarded to > dev mailing list. > > On Wed, Aug 16, 2017 at 10:15 AM, Patrick Hunt wrote: > > > On Wed, Aug 16, 2017 at 9:51 AM, Jordan Zimmerman < > > jor...@jordanzimmerman.com> wrote: > > > > > * Review other people's patch. If you help out, others will be more > > willing > > > to do the same for you. If someone is kind enough to review your code, > > you > > > should return the favor to for someone else. > > > > > > > > > That's fair - I should personally try to do more of this. I'll make an > > > effort here. > > > > > > > > It's not clear to me how we are identifying patches for review today. We > > used to have a very clear process - a jira needed to be in the "patch > > available" state in order to be considered for commit. > > > > See "contribute" section here, notice that it's watered down from what it > > used to be: > > https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute > > > > Dan's patch is not in "patch available" state, is that one of the reasons > > why it's not being moved forward? > > > > Patrick > > > > > > > -Jordan > > > > > > > > > -- > Cheers > Michael. >
Re: Process for reviewing submitted patches?
We are using github pull request instead of the old patch approach since last October. So the status of JIRA is irrelevant now (in particular, Patch Available will not trigger Jenkins pre-commit workflow now.). This was discussed on dev list when we moved to github, the thread's name is "[VOTE] move Apache Zookeeper to git". As for how to identify available patches for review, they should be all here: https://github.com/apache/zookeeper/pulls To get notified for new incoming pull requests: * Watch our github repo: https://github.com/apache/zookeeper Or * Subscribe to dev mailing list. Because we have git -> JIRA hook any new pull request will get cross posted to JIRA which will then be forwarded to dev mailing list. On Wed, Aug 16, 2017 at 10:15 AM, Patrick Huntwrote: > On Wed, Aug 16, 2017 at 9:51 AM, Jordan Zimmerman < > jor...@jordanzimmerman.com> wrote: > > > * Review other people's patch. If you help out, others will be more > willing > > to do the same for you. If someone is kind enough to review your code, > you > > should return the favor to for someone else. > > > > > > That's fair - I should personally try to do more of this. I'll make an > > effort here. > > > > > It's not clear to me how we are identifying patches for review today. We > used to have a very clear process - a jira needed to be in the "patch > available" state in order to be considered for commit. > > See "contribute" section here, notice that it's watered down from what it > used to be: > https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute > > Dan's patch is not in "patch available" state, is that one of the reasons > why it's not being moved forward? > > Patrick > > > > -Jordan > > > -- Cheers Michael.
Re: Process for reviewing submitted patches?
On Wed, Aug 16, 2017 at 9:51 AM, Jordan Zimmerman < jor...@jordanzimmerman.com> wrote: > * Review other people's patch. If you help out, others will be more willing > to do the same for you. If someone is kind enough to review your code, you > should return the favor to for someone else. > > > That's fair - I should personally try to do more of this. I'll make an > effort here. > > It's not clear to me how we are identifying patches for review today. We used to have a very clear process - a jira needed to be in the "patch available" state in order to be considered for commit. See "contribute" section here, notice that it's watered down from what it used to be: https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute Dan's patch is not in "patch available" state, is that one of the reasons why it's not being moved forward? Patrick > -Jordan >
Re: Process for reviewing submitted patches?
> * Review other people's patch. If you help out, others will be more willing > to do the same for you. If someone is kind enough to review your code, you > should return the favor to for someone else. That's fair - I should personally try to do more of this. I'll make an effort here. -Jordan smime.p7s Description: S/MIME cryptographic signature
Re: Process for reviewing submitted patches?
I have to agree with your sentiments. I don't want to overstate it - I'm involved with several OSS projects myself - but it does seem that ZooKeeper needs either more committers or more engagement from the existing committers. It's been very difficult to get traction on issues recently. I've had to be a pest to get responses. To be fair, if you keep at it eventually there is a response but I think it should be easier. To be clear, I know from personal experience how hard this is given that none of us get paid to do this and it's usually done in our spare time. -Jordan > On Aug 16, 2017, at 5:30 PM, Dan Benediktson >wrote: > > Hi there, > > Does the Zookeeper project have any formal process for ensuring submitted > patches get reviewed and subsequently committed? > > About a week ago I again submitted a patch for > https://issues.apache.org/jira/browse/ZOOKEEPER-2471. This is something > like the third time I've submitted a patch to Apache Zookeeper over the > past year, and none of them has ever been reviewed. While they have all > fixed real bugs we've seen in production while running Zookeeper, I have > never urgently needed them to be committed because we maintain a fork where > we have already taken the bug fixes we need, so I have been inclined to not > make a nuisance of myself and let the Zookeeper PMC decide the best course > of action, but this is honestly somewhat frustrating. I would much rather > run Apache Zookeeper than run a private fork of it, but given the > experience so far in pushing our patches upstream and the sheer number and > scope of patches we have, this is a pretty daunting thought right now. > > I realize this is a volunteer operation and that we all have day jobs, > but I feel like this situation needs some improvement. Would it be possible > for the committers to set up some sort of regular review cadence and > provide some sort of loose expected SLA for reviewing, and assuming review > is approved, subsequently committing, submitted patches? To be clear, I > don't want to push a lot of work or strict timelines or anything: like I > said, I realize this is a volunteer project and that we're all quite busy. > But if we could even get something like a 1-month intended SLA for > reviewing a submitted patch, and then a 1-month intended SLA for committing > after a patch was accepted in review, I think it would be hugely beneficial > for contributors. > > Thanks, > Dan smime.p7s Description: S/MIME cryptographic signature
Re: Process for reviewing submitted patches?
Thanks for bringing this issue up. I think it's an important issue for the ZooKeeper community. The fundamental issue here is that we don't have enough active code reviewers and committers, which limits the throughput of the code reviews, since a patch has to be reviewed and approved by at least one committer to land. With this constraint the SLA is likely not going to work, unless we grow the community by increasing code reviewers and committers. To improve the current situation, my thoughts are: * Any developers here should participate code reviews as both reviewer and reviewee. You don't need be a committer to do code reviews. * Review other people's patch. If you help out, others will be more willing to do the same for you. If someone is kind enough to review your code, you should return the favor to for someone else. * Ping the dev list on your patch. If it's urgent, provide reasons on why and then ping dev list every couple of days. If it's not urgent, ping dev list every one or two weeks. * Ping individual developers directly and / or privately for escalation. It's less likely such ping will be ignored. On growing new committer side, PMCs are actively working on bringing new blood who demonstrates passion and effort on helping out patch reviews, among other contributions. On Wed, Aug 16, 2017 at 8:30 AM, Dan Benediktsonwrote: > Hi there, > > Does the Zookeeper project have any formal process for ensuring submitted > patches get reviewed and subsequently committed? > > About a week ago I again submitted a patch for > https://issues.apache.org/jira/browse/ZOOKEEPER-2471. This is something > like the third time I've submitted a patch to Apache Zookeeper over the > past year, and none of them has ever been reviewed. While they have all > fixed real bugs we've seen in production while running Zookeeper, I have > never urgently needed them to be committed because we maintain a fork where > we have already taken the bug fixes we need, so I have been inclined to not > make a nuisance of myself and let the Zookeeper PMC decide the best course > of action, but this is honestly somewhat frustrating. I would much rather > run Apache Zookeeper than run a private fork of it, but given the > experience so far in pushing our patches upstream and the sheer number and > scope of patches we have, this is a pretty daunting thought right now. > > I realize this is a volunteer operation and that we all have day jobs, > but I feel like this situation needs some improvement. Would it be possible > for the committers to set up some sort of regular review cadence and > provide some sort of loose expected SLA for reviewing, and assuming review > is approved, subsequently committing, submitted patches? To be clear, I > don't want to push a lot of work or strict timelines or anything: like I > said, I realize this is a volunteer project and that we're all quite busy. > But if we could even get something like a 1-month intended SLA for > reviewing a submitted patch, and then a 1-month intended SLA for committing > after a patch was accepted in review, I think it would be hugely beneficial > for contributors. > > Thanks, > Dan > -- Cheers Michael.