You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Dan Benediktson <db...@twitter.com.INVALID> on 2017/08/16 15:30:53 UTC
Process for reviewing submitted patches?
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
Re: Process for reviewing submitted patches?
Posted by Jordan Zimmerman <jo...@jordanzimmerman.com>.
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 <db...@twitter.com.INVALID> 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
Re: Process for reviewing submitted patches?
Posted by Camille Fournier <ca...@apache.org>.
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 Han <ha...@cloudera.com> wrote:
> 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 <ph...@apache.org> wrote:
>
> > On Wed, Aug 16, 2017 at 9:51 AM, Jordan Zimmerman <
> > jordan@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?
Posted by Michael Han <ha...@cloudera.com>.
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 <ph...@apache.org> wrote:
> On Wed, Aug 16, 2017 at 9:51 AM, Jordan Zimmerman <
> jordan@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?
Posted by Patrick Hunt <ph...@apache.org>.
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 <
jordan@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 Hunt <ph...@apache.org> wrote:
> >
> > On Wed, Aug 16, 2017 at 9:51 AM, Jordan Zimmerman <
> > jordan@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?
Posted by Jordan Zimmerman <jo...@jordanzimmerman.com>.
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 Hunt <ph...@apache.org> wrote:
>
> On Wed, Aug 16, 2017 at 9:51 AM, Jordan Zimmerman <
> jordan@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?
Posted by Patrick Hunt <ph...@apache.org>.
On Wed, Aug 16, 2017 at 9:51 AM, Jordan Zimmerman <
jordan@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?
Posted by Jordan Zimmerman <jo...@jordanzimmerman.com>.
> * 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
Re: Process for reviewing submitted patches?
Posted by Michael Han <ha...@cloudera.com>.
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 Benediktson <dbenediktson@twitter.com.
invalid> 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
>
--
Cheers
Michael.