You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-dev@hadoop.apache.org by Andrew Wang <an...@cloudera.com> on 2015/01/26 20:55:22 UTC
Re: Patch review process
Let's move this over to common-dev@, general@ is normally used for project
announcements rather than discussion topics.
I'd like to summarize a few things mentioned on the private@ thread,
related to streamlining the code submission process.
- Gerrit was brought up again, as it has in the past, as something that
could make the actual process of reviewing and committing a lot easier.
This would be especially helpful for small patches, where the mechanics of
committing can take longer than reviewing the patch.
- There were also concerns about forking discussions between JIRA and
Gerrit. This has been an issue in Spark, and we'd like to keep discussions
and issue tracking centralized.
- Some talk about how to improve precommit. Right now it takes hours to run
the unit tests, which slows down patch iterations. One solution is running
tests in parallel (and even distributed). Previous distributed experiments
have done a full unit test run in a couple minutes, but it'd be a fair
amount of work to actually make this production ready.
- Also mention of putting in place more linting and static analysis.
Automating this will save reviewer time.
Best,
Andrew
On Mon, Jan 26, 2015 at 9:16 AM, Ted Yu <yu...@gmail.com> wrote:
> In some cases, contributor responded to review comments and attached
> patches addressing the comments.
>
> Later on, there was simply no response to the latest patch - even with
> follow-on ping.
>
> I wish this aspect can be improved.
>
> Cheers
>
> On Sun, Jan 25, 2015 at 6:03 PM, Tsz Wo (Nicholas), Sze <
> s29752-hadoopgeneral@yahoo.com.invalid> wrote:
>
> > Hi contributors,
> > I would like to (re)start a discussion regrading to our patch review
> > process. A similar discussion has been happened in a the hadoop private
> > mailing list, which is inappropriate.
> > Here is the problem:The patch available queues become longer and longer.
> > It seems that we never can catch up. There are patches sitting in the
> > queues for years. How could we speed up?
> > Regrads,Tsz-Wo
> >
>
Re: Patch review process
Posted by "Colin P. McCabe" <cm...@apache.org>.
I really do not think it's worth looking at Reviewboard at
reviews.apache.org again. We have used it in the past, and it has
all the downsides of gerrit and none of the upsides. And some extra
downsides of its own.
* Splits the conversation into two places
* No way to search the split out conversation (no sophisticated query
language like JQL)
* An additional thing that new contributors have to learn
* A barrier to non-coders (since they don't post patches and so can't
contribute to the discussion there).
* Clunky UI (in my opinion)
* Requires manually filling in a long HTML form to upload a patch, or
using a fragile uploader script that often breaks when the reviewboard
software is updated
* No way to press a button and have a patch committed... the patch
commit process is just as time-consuming as it is now.
Sorry, but -1.
I like the Crucible idea, though.
If we want to investigate alternatives to Crucible, how about looking
at gerrit? It has 1-click commits, integration with git (so that
posting a patch is just a single "git push"), and the potential to
mirror comments to JIRA (or at least someone said it might?)
Colin
On Tue, Jan 27, 2015 at 2:50 AM, Steve Loughran <st...@hortonworks.com> wrote:
> I'd be +1 on trying reviews.apache.org on a JIRA which
>
> 1. had multiple distributed people working on it
> 2. had some tangible code needing reviewing
> 3. was of limited enough size/duration that we'd see how well it worked
>
> do that, get feedback from the participants and repeat until we're happy
> with a process.
>
> if others can try cruicible at the same time, that would parallelise the
> work.
>
> On 26 January 2015 at 22:41, Chris Nauroth <cn...@hortonworks.com> wrote:
>
>> reviews.apache.org is backed by Review Board, and I've had a very positive
>> experience with that tool on other projects. HADOOP-9629 is a Hadoop patch
>> where we decided to go ahead and use it, and I think it helped. AFAIK,
>> there is no rule against using it in Hadoop, but it does have the side
>> effect of splitting part of the conversation out of jira. If Crucible can
>> keep all the review notes sync'd with the jira and searchable, then that
>> would be very interesting.
>>
>> Chris Nauroth
>> Hortonworks
>> http://hortonworks.com/
>>
>>
>> On Mon, Jan 26, 2015 at 1:54 PM, Arpit Agarwal <aa...@hortonworks.com>
>> wrote:
>>
>> > IMO the number one improvement would be a web-based review tool. We could
>> > evaluate Atlassian Crucible since it claims to integrate well with Jira.
>> I
>> > have not tried https://reviews.apache.org/r/new/.
>> >
>> > Some easy improvements that were also raised by others on the private
>> list:
>> > - Encourage contributors to batch related trivial fixes into a single
>> > patch.
>> > - Require more detailed descriptions with non-trivial patch
>> contributions.
>> > For patches that require knowledge of a specific subsystem a
>> > background+design note would be a good start.
>> > - Eliminate CHANGES.txt. This came up on the dev list not too long ago
>> and
>> > IIRC Allen did a PoC.
>> >
>> > I am not optimistic about Gerrit from past experience. It does help gate
>> > checkins and enforce pre-commit checks (good). I did not find it
>> > user-friendly and it will be an additional hurdle for contributors to
>> > understand (bad).
>> >
>> > Andrew, can the community build on your distributed pre-commit work to
>> make
>> > it production ready?
>> >
>> > Regards,
>> > Arpit
>> >
>> >
>> > On Mon, Jan 26, 2015 at 11:55 AM, Andrew Wang <an...@cloudera.com>
>> > wrote:
>> >
>> > > Let's move this over to common-dev@, general@ is normally used for
>> > project
>> > > announcements rather than discussion topics.
>> > >
>> > > I'd like to summarize a few things mentioned on the private@ thread,
>> > > related to streamlining the code submission process.
>> > >
>> > > - Gerrit was brought up again, as it has in the past, as something that
>> > > could make the actual process of reviewing and committing a lot easier.
>> > > This would be especially helpful for small patches, where the mechanics
>> > of
>> > > committing can take longer than reviewing the patch.
>> > > - There were also concerns about forking discussions between JIRA and
>> > > Gerrit. This has been an issue in Spark, and we'd like to keep
>> > discussions
>> > > and issue tracking centralized.
>> > >
>> > > - Some talk about how to improve precommit. Right now it takes hours to
>> > run
>> > > the unit tests, which slows down patch iterations. One solution is
>> > running
>> > > tests in parallel (and even distributed). Previous distributed
>> > experiments
>> > > have done a full unit test run in a couple minutes, but it'd be a fair
>> > > amount of work to actually make this production ready.
>> > > - Also mention of putting in place more linting and static analysis.
>> > > Automating this will save reviewer time.
>> > >
>> > > Best,
>> > > Andrew
>> > >
>> > > On Mon, Jan 26, 2015 at 9:16 AM, Ted Yu <yu...@gmail.com> wrote:
>> > >
>> > > > In some cases, contributor responded to review comments and attached
>> > > > patches addressing the comments.
>> > > >
>> > > > Later on, there was simply no response to the latest patch - even
>> with
>> > > > follow-on ping.
>> > > >
>> > > > I wish this aspect can be improved.
>> > > >
>> > > > Cheers
>> > > >
>> > > > On Sun, Jan 25, 2015 at 6:03 PM, Tsz Wo (Nicholas), Sze <
>> > > > s29752-hadoopgeneral@yahoo.com.invalid> wrote:
>> > > >
>> > > > > Hi contributors,
>> > > > > I would like to (re)start a discussion regrading to our patch
>> review
>> > > > > process. A similar discussion has been happened in a the hadoop
>> > > private
>> > > > > mailing list, which is inappropriate.
>> > > > > Here is the problem:The patch available queues become longer and
>> > > longer.
>> > > > > It seems that we never can catch up. There are patches sitting in
>> > the
>> > > > > queues for years. How could we speed up?
>> > > > > Regrads,Tsz-Wo
>> > > > >
>> > > >
>> > >
>> >
>> > --
>> > CONFIDENTIALITY NOTICE
>> > NOTICE: This message is intended for the use of the individual or entity
>> to
>> > which it is addressed and may contain information that is confidential,
>> > privileged and exempt from disclosure under applicable law. If the reader
>> > of this message is not the intended recipient, you are hereby notified
>> that
>> > any printing, copying, dissemination, distribution, disclosure or
>> > forwarding of this communication is strictly prohibited. If you have
>> > received this communication in error, please contact the sender
>> immediately
>> > and delete it from your system. Thank You.
>> >
>>
>> --
>> CONFIDENTIALITY NOTICE
>> NOTICE: This message is intended for the use of the individual or entity to
>> which it is addressed and may contain information that is confidential,
>> privileged and exempt from disclosure under applicable law. If the reader
>> of this message is not the intended recipient, you are hereby notified that
>> any printing, copying, dissemination, distribution, disclosure or
>> forwarding of this communication is strictly prohibited. If you have
>> received this communication in error, please contact the sender immediately
>> and delete it from your system. Thank You.
>>
>
> --
> CONFIDENTIALITY NOTICE
> NOTICE: This message is intended for the use of the individual or entity to
> which it is addressed and may contain information that is confidential,
> privileged and exempt from disclosure under applicable law. If the reader
> of this message is not the intended recipient, you are hereby notified that
> any printing, copying, dissemination, distribution, disclosure or
> forwarding of this communication is strictly prohibited. If you have
> received this communication in error, please contact the sender immediately
> and delete it from your system. Thank You.
Re: Patch review process
Posted by Arpit Agarwal <aa...@hortonworks.com>.
Filed INFRA-9071 to update reviews.apache.org working with the hadoop git
repo. Let's see if that works out.
Looks like Crucible is no go from infra -
https://issues.apache.org/jira/browse/INFRA-8430.
On Tue, Jan 27, 2015 at 2:50 AM, Steve Loughran <st...@hortonworks.com>
wrote:
> I'd be +1 on trying reviews.apache.org on a JIRA which
>
> 1. had multiple distributed people working on it
> 2. had some tangible code needing reviewing
> 3. was of limited enough size/duration that we'd see how well it worked
>
> do that, get feedback from the participants and repeat until we're happy
> with a process.
>
> if others can try cruicible at the same time, that would parallelise the
> work.
>
> On 26 January 2015 at 22:41, Chris Nauroth <cn...@hortonworks.com>
> wrote:
>
> > reviews.apache.org is backed by Review Board, and I've had a very
> positive
> > experience with that tool on other projects. HADOOP-9629 is a Hadoop
> patch
> > where we decided to go ahead and use it, and I think it helped. AFAIK,
> > there is no rule against using it in Hadoop, but it does have the side
> > effect of splitting part of the conversation out of jira. If Crucible
> can
> > keep all the review notes sync'd with the jira and searchable, then that
> > would be very interesting.
> >
> > Chris Nauroth
> > Hortonworks
> > http://hortonworks.com/
> >
> >
> > On Mon, Jan 26, 2015 at 1:54 PM, Arpit Agarwal <aagarwal@hortonworks.com
> >
> > wrote:
> >
> > > IMO the number one improvement would be a web-based review tool. We
> could
> > > evaluate Atlassian Crucible since it claims to integrate well with
> Jira.
> > I
> > > have not tried https://reviews.apache.org/r/new/.
> > >
> > > Some easy improvements that were also raised by others on the private
> > list:
> > > - Encourage contributors to batch related trivial fixes into a single
> > > patch.
> > > - Require more detailed descriptions with non-trivial patch
> > contributions.
> > > For patches that require knowledge of a specific subsystem a
> > > background+design note would be a good start.
> > > - Eliminate CHANGES.txt. This came up on the dev list not too long ago
> > and
> > > IIRC Allen did a PoC.
> > >
> > > I am not optimistic about Gerrit from past experience. It does help
> gate
> > > checkins and enforce pre-commit checks (good). I did not find it
> > > user-friendly and it will be an additional hurdle for contributors to
> > > understand (bad).
> > >
> > > Andrew, can the community build on your distributed pre-commit work to
> > make
> > > it production ready?
> > >
> > > Regards,
> > > Arpit
> > >
> > >
> > > On Mon, Jan 26, 2015 at 11:55 AM, Andrew Wang <
> andrew.wang@cloudera.com>
> > > wrote:
> > >
> > > > Let's move this over to common-dev@, general@ is normally used for
> > > project
> > > > announcements rather than discussion topics.
> > > >
> > > > I'd like to summarize a few things mentioned on the private@ thread,
> > > > related to streamlining the code submission process.
> > > >
> > > > - Gerrit was brought up again, as it has in the past, as something
> that
> > > > could make the actual process of reviewing and committing a lot
> easier.
> > > > This would be especially helpful for small patches, where the
> mechanics
> > > of
> > > > committing can take longer than reviewing the patch.
> > > > - There were also concerns about forking discussions between JIRA and
> > > > Gerrit. This has been an issue in Spark, and we'd like to keep
> > > discussions
> > > > and issue tracking centralized.
> > > >
> > > > - Some talk about how to improve precommit. Right now it takes hours
> to
> > > run
> > > > the unit tests, which slows down patch iterations. One solution is
> > > running
> > > > tests in parallel (and even distributed). Previous distributed
> > > experiments
> > > > have done a full unit test run in a couple minutes, but it'd be a
> fair
> > > > amount of work to actually make this production ready.
> > > > - Also mention of putting in place more linting and static analysis.
> > > > Automating this will save reviewer time.
> > > >
> > > > Best,
> > > > Andrew
> > > >
> > > > On Mon, Jan 26, 2015 at 9:16 AM, Ted Yu <yu...@gmail.com> wrote:
> > > >
> > > > > In some cases, contributor responded to review comments and
> attached
> > > > > patches addressing the comments.
> > > > >
> > > > > Later on, there was simply no response to the latest patch - even
> > with
> > > > > follow-on ping.
> > > > >
> > > > > I wish this aspect can be improved.
> > > > >
> > > > > Cheers
> > > > >
> > > > > On Sun, Jan 25, 2015 at 6:03 PM, Tsz Wo (Nicholas), Sze <
> > > > > s29752-hadoopgeneral@yahoo.com.invalid> wrote:
> > > > >
> > > > > > Hi contributors,
> > > > > > I would like to (re)start a discussion regrading to our patch
> > review
> > > > > > process. A similar discussion has been happened in a the hadoop
> > > > private
> > > > > > mailing list, which is inappropriate.
> > > > > > Here is the problem:The patch available queues become longer and
> > > > longer.
> > > > > > It seems that we never can catch up. There are patches sitting
> in
> > > the
> > > > > > queues for years. How could we speed up?
> > > > > > Regrads,Tsz-Wo
> > > > > >
> > > > >
> > > >
> > >
> > > --
> > > CONFIDENTIALITY NOTICE
> > > NOTICE: This message is intended for the use of the individual or
> entity
> > to
> > > which it is addressed and may contain information that is confidential,
> > > privileged and exempt from disclosure under applicable law. If the
> reader
> > > of this message is not the intended recipient, you are hereby notified
> > that
> > > any printing, copying, dissemination, distribution, disclosure or
> > > forwarding of this communication is strictly prohibited. If you have
> > > received this communication in error, please contact the sender
> > immediately
> > > and delete it from your system. Thank You.
> > >
> >
> > --
> > CONFIDENTIALITY NOTICE
> > NOTICE: This message is intended for the use of the individual or entity
> to
> > which it is addressed and may contain information that is confidential,
> > privileged and exempt from disclosure under applicable law. If the reader
> > of this message is not the intended recipient, you are hereby notified
> that
> > any printing, copying, dissemination, distribution, disclosure or
> > forwarding of this communication is strictly prohibited. If you have
> > received this communication in error, please contact the sender
> immediately
> > and delete it from your system. Thank You.
> >
>
> --
> CONFIDENTIALITY NOTICE
> NOTICE: This message is intended for the use of the individual or entity to
> which it is addressed and may contain information that is confidential,
> privileged and exempt from disclosure under applicable law. If the reader
> of this message is not the intended recipient, you are hereby notified that
> any printing, copying, dissemination, distribution, disclosure or
> forwarding of this communication is strictly prohibited. If you have
> received this communication in error, please contact the sender immediately
> and delete it from your system. Thank You.
>
--
CONFIDENTIALITY NOTICE
NOTICE: This message is intended for the use of the individual or entity to
which it is addressed and may contain information that is confidential,
privileged and exempt from disclosure under applicable law. If the reader
of this message is not the intended recipient, you are hereby notified that
any printing, copying, dissemination, distribution, disclosure or
forwarding of this communication is strictly prohibited. If you have
received this communication in error, please contact the sender immediately
and delete it from your system. Thank You.
Re: Patch review process
Posted by Steve Loughran <st...@hortonworks.com>.
I'd be +1 on trying reviews.apache.org on a JIRA which
1. had multiple distributed people working on it
2. had some tangible code needing reviewing
3. was of limited enough size/duration that we'd see how well it worked
do that, get feedback from the participants and repeat until we're happy
with a process.
if others can try cruicible at the same time, that would parallelise the
work.
On 26 January 2015 at 22:41, Chris Nauroth <cn...@hortonworks.com> wrote:
> reviews.apache.org is backed by Review Board, and I've had a very positive
> experience with that tool on other projects. HADOOP-9629 is a Hadoop patch
> where we decided to go ahead and use it, and I think it helped. AFAIK,
> there is no rule against using it in Hadoop, but it does have the side
> effect of splitting part of the conversation out of jira. If Crucible can
> keep all the review notes sync'd with the jira and searchable, then that
> would be very interesting.
>
> Chris Nauroth
> Hortonworks
> http://hortonworks.com/
>
>
> On Mon, Jan 26, 2015 at 1:54 PM, Arpit Agarwal <aa...@hortonworks.com>
> wrote:
>
> > IMO the number one improvement would be a web-based review tool. We could
> > evaluate Atlassian Crucible since it claims to integrate well with Jira.
> I
> > have not tried https://reviews.apache.org/r/new/.
> >
> > Some easy improvements that were also raised by others on the private
> list:
> > - Encourage contributors to batch related trivial fixes into a single
> > patch.
> > - Require more detailed descriptions with non-trivial patch
> contributions.
> > For patches that require knowledge of a specific subsystem a
> > background+design note would be a good start.
> > - Eliminate CHANGES.txt. This came up on the dev list not too long ago
> and
> > IIRC Allen did a PoC.
> >
> > I am not optimistic about Gerrit from past experience. It does help gate
> > checkins and enforce pre-commit checks (good). I did not find it
> > user-friendly and it will be an additional hurdle for contributors to
> > understand (bad).
> >
> > Andrew, can the community build on your distributed pre-commit work to
> make
> > it production ready?
> >
> > Regards,
> > Arpit
> >
> >
> > On Mon, Jan 26, 2015 at 11:55 AM, Andrew Wang <an...@cloudera.com>
> > wrote:
> >
> > > Let's move this over to common-dev@, general@ is normally used for
> > project
> > > announcements rather than discussion topics.
> > >
> > > I'd like to summarize a few things mentioned on the private@ thread,
> > > related to streamlining the code submission process.
> > >
> > > - Gerrit was brought up again, as it has in the past, as something that
> > > could make the actual process of reviewing and committing a lot easier.
> > > This would be especially helpful for small patches, where the mechanics
> > of
> > > committing can take longer than reviewing the patch.
> > > - There were also concerns about forking discussions between JIRA and
> > > Gerrit. This has been an issue in Spark, and we'd like to keep
> > discussions
> > > and issue tracking centralized.
> > >
> > > - Some talk about how to improve precommit. Right now it takes hours to
> > run
> > > the unit tests, which slows down patch iterations. One solution is
> > running
> > > tests in parallel (and even distributed). Previous distributed
> > experiments
> > > have done a full unit test run in a couple minutes, but it'd be a fair
> > > amount of work to actually make this production ready.
> > > - Also mention of putting in place more linting and static analysis.
> > > Automating this will save reviewer time.
> > >
> > > Best,
> > > Andrew
> > >
> > > On Mon, Jan 26, 2015 at 9:16 AM, Ted Yu <yu...@gmail.com> wrote:
> > >
> > > > In some cases, contributor responded to review comments and attached
> > > > patches addressing the comments.
> > > >
> > > > Later on, there was simply no response to the latest patch - even
> with
> > > > follow-on ping.
> > > >
> > > > I wish this aspect can be improved.
> > > >
> > > > Cheers
> > > >
> > > > On Sun, Jan 25, 2015 at 6:03 PM, Tsz Wo (Nicholas), Sze <
> > > > s29752-hadoopgeneral@yahoo.com.invalid> wrote:
> > > >
> > > > > Hi contributors,
> > > > > I would like to (re)start a discussion regrading to our patch
> review
> > > > > process. A similar discussion has been happened in a the hadoop
> > > private
> > > > > mailing list, which is inappropriate.
> > > > > Here is the problem:The patch available queues become longer and
> > > longer.
> > > > > It seems that we never can catch up. There are patches sitting in
> > the
> > > > > queues for years. How could we speed up?
> > > > > Regrads,Tsz-Wo
> > > > >
> > > >
> > >
> >
> > --
> > CONFIDENTIALITY NOTICE
> > NOTICE: This message is intended for the use of the individual or entity
> to
> > which it is addressed and may contain information that is confidential,
> > privileged and exempt from disclosure under applicable law. If the reader
> > of this message is not the intended recipient, you are hereby notified
> that
> > any printing, copying, dissemination, distribution, disclosure or
> > forwarding of this communication is strictly prohibited. If you have
> > received this communication in error, please contact the sender
> immediately
> > and delete it from your system. Thank You.
> >
>
> --
> CONFIDENTIALITY NOTICE
> NOTICE: This message is intended for the use of the individual or entity to
> which it is addressed and may contain information that is confidential,
> privileged and exempt from disclosure under applicable law. If the reader
> of this message is not the intended recipient, you are hereby notified that
> any printing, copying, dissemination, distribution, disclosure or
> forwarding of this communication is strictly prohibited. If you have
> received this communication in error, please contact the sender immediately
> and delete it from your system. Thank You.
>
--
CONFIDENTIALITY NOTICE
NOTICE: This message is intended for the use of the individual or entity to
which it is addressed and may contain information that is confidential,
privileged and exempt from disclosure under applicable law. If the reader
of this message is not the intended recipient, you are hereby notified that
any printing, copying, dissemination, distribution, disclosure or
forwarding of this communication is strictly prohibited. If you have
received this communication in error, please contact the sender immediately
and delete it from your system. Thank You.
Re: Patch review process
Posted by Sangjin Lee <sj...@gmail.com>.
As popular as the github workflow is, there are some things I'm not a big
fan of. For one, the review tends to cause a flurry of separate
notifications as opposed to a single atomic set of comments.
On Mon, Jan 26, 2015 at 6:17 PM, Arpit Agarwal <aa...@hortonworks.com>
wrote:
> I think someone mentioned this earlier - the concern was keeping the review
> comments searchable from one location, ideally within Jira.
>
>
> On Mon, Jan 26, 2015 at 5:52 PM, Ravi Prakash <ra...@ymail.com> wrote:
>
> > Just out of left field: Since we already migrated to git (Thanks everyone
> > for that effort) should we try using github's UI? Isn't that how the
> > majority of the rest of the world started doing code reviews?
> >
> >
> > On Monday, January 26, 2015 3:57 PM, Arpit Agarwal <
> > aagarwal@hortonworks.com> wrote:
> >
> >
> > Thanks for that data point Chris.
> >
> > It looks like reviews.apache.org no longer works. hadoop-hdfs-git may be
> > pointing to an outdated repository. I'll file a ticket with Infra.
> >
> > On Mon, Jan 26, 2015 at 2:41 PM, Chris Nauroth <cnauroth@hortonworks.com
> >
> > wrote:
> >
> > > reviews.apache.org is backed by Review Board, and I've had a very
> > positive
> > > experience with that tool on other projects. HADOOP-9629 is a Hadoop
> > patch
> > > where we decided to go ahead and use it, and I think it helped. AFAIK,
> > > there is no rule against using it in Hadoop, but it does have the side
> > > effect of splitting part of the conversation out of jira. If Crucible
> > can
> > > keep all the review notes sync'd with the jira and searchable, then
> that
> > > would be very interesting.
> > >
> > > Chris Nauroth
> > > Hortonworks
> > > http://hortonworks.com/
> > >
> > >
> > > On Mon, Jan 26, 2015 at 1:54 PM, Arpit Agarwal <
> aagarwal@hortonworks.com
> > >
> > > wrote:
> > >
> > > > IMO the number one improvement would be a web-based review tool. We
> > could
> > > > evaluate Atlassian Crucible since it claims to integrate well with
> > Jira.
> > > I
> > > > have not tried https://reviews.apache.org/r/new/.
> > > >
> > > > Some easy improvements that were also raised by others on the private
> > > list:
> > > > - Encourage contributors to batch related trivial fixes into a single
> > > > patch.
> > > > - Require more detailed descriptions with non-trivial patch
> > > contributions.
> > > > For patches that require knowledge of a specific subsystem a
> > > > background+design note would be a good start.
> > > > - Eliminate CHANGES.txt. This came up on the dev list not too long
> ago
> > > and
> > > > IIRC Allen did a PoC.
> > > >
> > > > I am not optimistic about Gerrit from past experience. It does help
> > gate
> > > > checkins and enforce pre-commit checks (good). I did not find it
> > > > user-friendly and it will be an additional hurdle for contributors to
> > > > understand (bad).
> > > >
> > > > Andrew, can the community build on your distributed pre-commit work
> to
> > > make
> > > > it production ready?
> > > >
> > > > Regards,
> > > > Arpit
> > > >
> > > >
> > > > On Mon, Jan 26, 2015 at 11:55 AM, Andrew Wang <
> > andrew.wang@cloudera.com>
> > > > wrote:
> > > >
> > > > > Let's move this over to common-dev@, general@ is normally used for
> > > > project
> > > > > announcements rather than discussion topics.
> > > > >
> > > > > I'd like to summarize a few things mentioned on the private@
> thread,
> > > > > related to streamlining the code submission process.
> > > > >
> > > > > - Gerrit was brought up again, as it has in the past, as something
> > that
> > > > > could make the actual process of reviewing and committing a lot
> > easier.
> > > > > This would be especially helpful for small patches, where the
> > mechanics
> > > > of
> > > > > committing can take longer than reviewing the patch.
> > > > > - There were also concerns about forking discussions between JIRA
> and
> > > > > Gerrit. This has been an issue in Spark, and we'd like to keep
> > > > discussions
> > > > > and issue tracking centralized.
> > > > >
> > > > > - Some talk about how to improve precommit. Right now it takes
> hours
> > to
> > > > run
> > > > > the unit tests, which slows down patch iterations. One solution is
> > > > running
> > > > > tests in parallel (and even distributed). Previous distributed
> > > > experiments
> > > > > have done a full unit test run in a couple minutes, but it'd be a
> > fair
> > > > > amount of work to actually make this production ready.
> > > > > - Also mention of putting in place more linting and static
> analysis.
> > > > > Automating this will save reviewer time.
> > > > >
> > > > > Best,
> > > > > Andrew
> > > > >
> > > > > On Mon, Jan 26, 2015 at 9:16 AM, Ted Yu <yu...@gmail.com>
> wrote:
> > > > >
> > > > > > In some cases, contributor responded to review comments and
> > attached
> > > > > > patches addressing the comments.
> > > > > >
> > > > > > Later on, there was simply no response to the latest patch - even
> > > with
> > > > > > follow-on ping.
> > > > > >
> > > > > > I wish this aspect can be improved.
> > > > > >
> > > > > > Cheers
> > > > > >
> > > > > > On Sun, Jan 25, 2015 at 6:03 PM, Tsz Wo (Nicholas), Sze <
> > > > > > s29752-hadoopgeneral@yahoo.com.invalid> wrote:
> > > > > >
> > > > > > > Hi contributors,
> > > > > > > I would like to (re)start a discussion regrading to our patch
> > > review
> > > > > > > process. A similar discussion has been happened in a the
> hadoop
> > > > > private
> > > > > > > mailing list, which is inappropriate.
> > > > > > > Here is the problem:The patch available queues become longer
> and
> > > > > longer.
> > > > > > > It seems that we never can catch up. There are patches sitting
> > in
> > > > the
> > > > > > > queues for years. How could we speed up?
> > > > > > > Regrads,Tsz-Wo
> > > > > > >
> > > > > >
> > > > >
> > > >
> > > > --
> > > > CONFIDENTIALITY NOTICE
> > > > NOTICE: This message is intended for the use of the individual or
> > entity
> > > to
> > > > which it is addressed and may contain information that is
> confidential,
> > > > privileged and exempt from disclosure under applicable law. If the
> > reader
> > > > of this message is not the intended recipient, you are hereby
> notified
> > > that
> > > > any printing, copying, dissemination, distribution, disclosure or
> > > > forwarding of this communication is strictly prohibited. If you have
> > > > received this communication in error, please contact the sender
> > > immediately
> > > > and delete it from your system. Thank You.
> > > >
> > >
> > > --
> > > CONFIDENTIALITY NOTICE
> > > NOTICE: This message is intended for the use of the individual or
> entity
> > to
> > > which it is addressed and may contain information that is confidential,
> > > privileged and exempt from disclosure under applicable law. If the
> reader
> > > of this message is not the intended recipient, you are hereby notified
> > that
> > > any printing, copying, dissemination, distribution, disclosure or
> > > forwarding of this communication is strictly prohibited. If you have
> > > received this communication in error, please contact the sender
> > immediately
> > > and delete it from your system. Thank You.
> > >
> >
> > --
> > CONFIDENTIALITY NOTICE
> > NOTICE: This message is intended for the use of the individual or entity
> to
> > which it is addressed and may contain information that is confidential,
> > privileged and exempt from disclosure under applicable law. If the reader
> > of this message is not the intended recipient, you are hereby notified
> that
> > any printing, copying, dissemination, distribution, disclosure or
> > forwarding of this communication is strictly prohibited. If you have
> > received this communication in error, please contact the sender
> immediately
> > and delete it from your system. Thank You.
> >
> >
> >
> >
>
> --
> CONFIDENTIALITY NOTICE
> NOTICE: This message is intended for the use of the individual or entity to
> which it is addressed and may contain information that is confidential,
> privileged and exempt from disclosure under applicable law. If the reader
> of this message is not the intended recipient, you are hereby notified that
> any printing, copying, dissemination, distribution, disclosure or
> forwarding of this communication is strictly prohibited. If you have
> received this communication in error, please contact the sender immediately
> and delete it from your system. Thank You.
>
Re: Patch review process
Posted by Arpit Agarwal <aa...@hortonworks.com>.
I think someone mentioned this earlier - the concern was keeping the review
comments searchable from one location, ideally within Jira.
On Mon, Jan 26, 2015 at 5:52 PM, Ravi Prakash <ra...@ymail.com> wrote:
> Just out of left field: Since we already migrated to git (Thanks everyone
> for that effort) should we try using github's UI? Isn't that how the
> majority of the rest of the world started doing code reviews?
>
>
> On Monday, January 26, 2015 3:57 PM, Arpit Agarwal <
> aagarwal@hortonworks.com> wrote:
>
>
> Thanks for that data point Chris.
>
> It looks like reviews.apache.org no longer works. hadoop-hdfs-git may be
> pointing to an outdated repository. I'll file a ticket with Infra.
>
> On Mon, Jan 26, 2015 at 2:41 PM, Chris Nauroth <cn...@hortonworks.com>
> wrote:
>
> > reviews.apache.org is backed by Review Board, and I've had a very
> positive
> > experience with that tool on other projects. HADOOP-9629 is a Hadoop
> patch
> > where we decided to go ahead and use it, and I think it helped. AFAIK,
> > there is no rule against using it in Hadoop, but it does have the side
> > effect of splitting part of the conversation out of jira. If Crucible
> can
> > keep all the review notes sync'd with the jira and searchable, then that
> > would be very interesting.
> >
> > Chris Nauroth
> > Hortonworks
> > http://hortonworks.com/
> >
> >
> > On Mon, Jan 26, 2015 at 1:54 PM, Arpit Agarwal <aagarwal@hortonworks.com
> >
> > wrote:
> >
> > > IMO the number one improvement would be a web-based review tool. We
> could
> > > evaluate Atlassian Crucible since it claims to integrate well with
> Jira.
> > I
> > > have not tried https://reviews.apache.org/r/new/.
> > >
> > > Some easy improvements that were also raised by others on the private
> > list:
> > > - Encourage contributors to batch related trivial fixes into a single
> > > patch.
> > > - Require more detailed descriptions with non-trivial patch
> > contributions.
> > > For patches that require knowledge of a specific subsystem a
> > > background+design note would be a good start.
> > > - Eliminate CHANGES.txt. This came up on the dev list not too long ago
> > and
> > > IIRC Allen did a PoC.
> > >
> > > I am not optimistic about Gerrit from past experience. It does help
> gate
> > > checkins and enforce pre-commit checks (good). I did not find it
> > > user-friendly and it will be an additional hurdle for contributors to
> > > understand (bad).
> > >
> > > Andrew, can the community build on your distributed pre-commit work to
> > make
> > > it production ready?
> > >
> > > Regards,
> > > Arpit
> > >
> > >
> > > On Mon, Jan 26, 2015 at 11:55 AM, Andrew Wang <
> andrew.wang@cloudera.com>
> > > wrote:
> > >
> > > > Let's move this over to common-dev@, general@ is normally used for
> > > project
> > > > announcements rather than discussion topics.
> > > >
> > > > I'd like to summarize a few things mentioned on the private@ thread,
> > > > related to streamlining the code submission process.
> > > >
> > > > - Gerrit was brought up again, as it has in the past, as something
> that
> > > > could make the actual process of reviewing and committing a lot
> easier.
> > > > This would be especially helpful for small patches, where the
> mechanics
> > > of
> > > > committing can take longer than reviewing the patch.
> > > > - There were also concerns about forking discussions between JIRA and
> > > > Gerrit. This has been an issue in Spark, and we'd like to keep
> > > discussions
> > > > and issue tracking centralized.
> > > >
> > > > - Some talk about how to improve precommit. Right now it takes hours
> to
> > > run
> > > > the unit tests, which slows down patch iterations. One solution is
> > > running
> > > > tests in parallel (and even distributed). Previous distributed
> > > experiments
> > > > have done a full unit test run in a couple minutes, but it'd be a
> fair
> > > > amount of work to actually make this production ready.
> > > > - Also mention of putting in place more linting and static analysis.
> > > > Automating this will save reviewer time.
> > > >
> > > > Best,
> > > > Andrew
> > > >
> > > > On Mon, Jan 26, 2015 at 9:16 AM, Ted Yu <yu...@gmail.com> wrote:
> > > >
> > > > > In some cases, contributor responded to review comments and
> attached
> > > > > patches addressing the comments.
> > > > >
> > > > > Later on, there was simply no response to the latest patch - even
> > with
> > > > > follow-on ping.
> > > > >
> > > > > I wish this aspect can be improved.
> > > > >
> > > > > Cheers
> > > > >
> > > > > On Sun, Jan 25, 2015 at 6:03 PM, Tsz Wo (Nicholas), Sze <
> > > > > s29752-hadoopgeneral@yahoo.com.invalid> wrote:
> > > > >
> > > > > > Hi contributors,
> > > > > > I would like to (re)start a discussion regrading to our patch
> > review
> > > > > > process. A similar discussion has been happened in a the hadoop
> > > > private
> > > > > > mailing list, which is inappropriate.
> > > > > > Here is the problem:The patch available queues become longer and
> > > > longer.
> > > > > > It seems that we never can catch up. There are patches sitting
> in
> > > the
> > > > > > queues for years. How could we speed up?
> > > > > > Regrads,Tsz-Wo
> > > > > >
> > > > >
> > > >
> > >
> > > --
> > > CONFIDENTIALITY NOTICE
> > > NOTICE: This message is intended for the use of the individual or
> entity
> > to
> > > which it is addressed and may contain information that is confidential,
> > > privileged and exempt from disclosure under applicable law. If the
> reader
> > > of this message is not the intended recipient, you are hereby notified
> > that
> > > any printing, copying, dissemination, distribution, disclosure or
> > > forwarding of this communication is strictly prohibited. If you have
> > > received this communication in error, please contact the sender
> > immediately
> > > and delete it from your system. Thank You.
> > >
> >
> > --
> > CONFIDENTIALITY NOTICE
> > NOTICE: This message is intended for the use of the individual or entity
> to
> > which it is addressed and may contain information that is confidential,
> > privileged and exempt from disclosure under applicable law. If the reader
> > of this message is not the intended recipient, you are hereby notified
> that
> > any printing, copying, dissemination, distribution, disclosure or
> > forwarding of this communication is strictly prohibited. If you have
> > received this communication in error, please contact the sender
> immediately
> > and delete it from your system. Thank You.
> >
>
> --
> CONFIDENTIALITY NOTICE
> NOTICE: This message is intended for the use of the individual or entity to
> which it is addressed and may contain information that is confidential,
> privileged and exempt from disclosure under applicable law. If the reader
> of this message is not the intended recipient, you are hereby notified that
> any printing, copying, dissemination, distribution, disclosure or
> forwarding of this communication is strictly prohibited. If you have
> received this communication in error, please contact the sender immediately
> and delete it from your system. Thank You.
>
>
>
>
--
CONFIDENTIALITY NOTICE
NOTICE: This message is intended for the use of the individual or entity to
which it is addressed and may contain information that is confidential,
privileged and exempt from disclosure under applicable law. If the reader
of this message is not the intended recipient, you are hereby notified that
any printing, copying, dissemination, distribution, disclosure or
forwarding of this communication is strictly prohibited. If you have
received this communication in error, please contact the sender immediately
and delete it from your system. Thank You.
Re: Patch review process
Posted by Ravi Prakash <ra...@ymail.com>.
Just out of left field: Since we already migrated to git (Thanks everyone for that effort) should we try using github's UI? Isn't that how the majority of the rest of the world started doing code reviews?
On Monday, January 26, 2015 3:57 PM, Arpit Agarwal <aa...@hortonworks.com> wrote:
Thanks for that data point Chris.
It looks like reviews.apache.org no longer works. hadoop-hdfs-git may be
pointing to an outdated repository. I'll file a ticket with Infra.
On Mon, Jan 26, 2015 at 2:41 PM, Chris Nauroth <cn...@hortonworks.com>
wrote:
> reviews.apache.org is backed by Review Board, and I've had a very positive
> experience with that tool on other projects. HADOOP-9629 is a Hadoop patch
> where we decided to go ahead and use it, and I think it helped. AFAIK,
> there is no rule against using it in Hadoop, but it does have the side
> effect of splitting part of the conversation out of jira. If Crucible can
> keep all the review notes sync'd with the jira and searchable, then that
> would be very interesting.
>
> Chris Nauroth
> Hortonworks
> http://hortonworks.com/
>
>
> On Mon, Jan 26, 2015 at 1:54 PM, Arpit Agarwal <aa...@hortonworks.com>
> wrote:
>
> > IMO the number one improvement would be a web-based review tool. We could
> > evaluate Atlassian Crucible since it claims to integrate well with Jira.
> I
> > have not tried https://reviews.apache.org/r/new/.
> >
> > Some easy improvements that were also raised by others on the private
> list:
> > - Encourage contributors to batch related trivial fixes into a single
> > patch.
> > - Require more detailed descriptions with non-trivial patch
> contributions.
> > For patches that require knowledge of a specific subsystem a
> > background+design note would be a good start.
> > - Eliminate CHANGES.txt. This came up on the dev list not too long ago
> and
> > IIRC Allen did a PoC.
> >
> > I am not optimistic about Gerrit from past experience. It does help gate
> > checkins and enforce pre-commit checks (good). I did not find it
> > user-friendly and it will be an additional hurdle for contributors to
> > understand (bad).
> >
> > Andrew, can the community build on your distributed pre-commit work to
> make
> > it production ready?
> >
> > Regards,
> > Arpit
> >
> >
> > On Mon, Jan 26, 2015 at 11:55 AM, Andrew Wang <an...@cloudera.com>
> > wrote:
> >
> > > Let's move this over to common-dev@, general@ is normally used for
> > project
> > > announcements rather than discussion topics.
> > >
> > > I'd like to summarize a few things mentioned on the private@ thread,
> > > related to streamlining the code submission process.
> > >
> > > - Gerrit was brought up again, as it has in the past, as something that
> > > could make the actual process of reviewing and committing a lot easier.
> > > This would be especially helpful for small patches, where the mechanics
> > of
> > > committing can take longer than reviewing the patch.
> > > - There were also concerns about forking discussions between JIRA and
> > > Gerrit. This has been an issue in Spark, and we'd like to keep
> > discussions
> > > and issue tracking centralized.
> > >
> > > - Some talk about how to improve precommit. Right now it takes hours to
> > run
> > > the unit tests, which slows down patch iterations. One solution is
> > running
> > > tests in parallel (and even distributed). Previous distributed
> > experiments
> > > have done a full unit test run in a couple minutes, but it'd be a fair
> > > amount of work to actually make this production ready.
> > > - Also mention of putting in place more linting and static analysis.
> > > Automating this will save reviewer time.
> > >
> > > Best,
> > > Andrew
> > >
> > > On Mon, Jan 26, 2015 at 9:16 AM, Ted Yu <yu...@gmail.com> wrote:
> > >
> > > > In some cases, contributor responded to review comments and attached
> > > > patches addressing the comments.
> > > >
> > > > Later on, there was simply no response to the latest patch - even
> with
> > > > follow-on ping.
> > > >
> > > > I wish this aspect can be improved.
> > > >
> > > > Cheers
> > > >
> > > > On Sun, Jan 25, 2015 at 6:03 PM, Tsz Wo (Nicholas), Sze <
> > > > s29752-hadoopgeneral@yahoo.com.invalid> wrote:
> > > >
> > > > > Hi contributors,
> > > > > I would like to (re)start a discussion regrading to our patch
> review
> > > > > process. A similar discussion has been happened in a the hadoop
> > > private
> > > > > mailing list, which is inappropriate.
> > > > > Here is the problem:The patch available queues become longer and
> > > longer.
> > > > > It seems that we never can catch up. There are patches sitting in
> > the
> > > > > queues for years. How could we speed up?
> > > > > Regrads,Tsz-Wo
> > > > >
> > > >
> > >
> >
> > --
> > CONFIDENTIALITY NOTICE
> > NOTICE: This message is intended for the use of the individual or entity
> to
> > which it is addressed and may contain information that is confidential,
> > privileged and exempt from disclosure under applicable law. If the reader
> > of this message is not the intended recipient, you are hereby notified
> that
> > any printing, copying, dissemination, distribution, disclosure or
> > forwarding of this communication is strictly prohibited. If you have
> > received this communication in error, please contact the sender
> immediately
> > and delete it from your system. Thank You.
> >
>
> --
> CONFIDENTIALITY NOTICE
> NOTICE: This message is intended for the use of the individual or entity to
> which it is addressed and may contain information that is confidential,
> privileged and exempt from disclosure under applicable law. If the reader
> of this message is not the intended recipient, you are hereby notified that
> any printing, copying, dissemination, distribution, disclosure or
> forwarding of this communication is strictly prohibited. If you have
> received this communication in error, please contact the sender immediately
> and delete it from your system. Thank You.
>
--
CONFIDENTIALITY NOTICE
NOTICE: This message is intended for the use of the individual or entity to
which it is addressed and may contain information that is confidential,
privileged and exempt from disclosure under applicable law. If the reader
of this message is not the intended recipient, you are hereby notified that
any printing, copying, dissemination, distribution, disclosure or
forwarding of this communication is strictly prohibited. If you have
received this communication in error, please contact the sender immediately
and delete it from your system. Thank You.
Re: Patch review process
Posted by Arpit Agarwal <aa...@hortonworks.com>.
Thanks for that data point Chris.
It looks like reviews.apache.org no longer works. hadoop-hdfs-git may be
pointing to an outdated repository. I'll file a ticket with Infra.
On Mon, Jan 26, 2015 at 2:41 PM, Chris Nauroth <cn...@hortonworks.com>
wrote:
> reviews.apache.org is backed by Review Board, and I've had a very positive
> experience with that tool on other projects. HADOOP-9629 is a Hadoop patch
> where we decided to go ahead and use it, and I think it helped. AFAIK,
> there is no rule against using it in Hadoop, but it does have the side
> effect of splitting part of the conversation out of jira. If Crucible can
> keep all the review notes sync'd with the jira and searchable, then that
> would be very interesting.
>
> Chris Nauroth
> Hortonworks
> http://hortonworks.com/
>
>
> On Mon, Jan 26, 2015 at 1:54 PM, Arpit Agarwal <aa...@hortonworks.com>
> wrote:
>
> > IMO the number one improvement would be a web-based review tool. We could
> > evaluate Atlassian Crucible since it claims to integrate well with Jira.
> I
> > have not tried https://reviews.apache.org/r/new/.
> >
> > Some easy improvements that were also raised by others on the private
> list:
> > - Encourage contributors to batch related trivial fixes into a single
> > patch.
> > - Require more detailed descriptions with non-trivial patch
> contributions.
> > For patches that require knowledge of a specific subsystem a
> > background+design note would be a good start.
> > - Eliminate CHANGES.txt. This came up on the dev list not too long ago
> and
> > IIRC Allen did a PoC.
> >
> > I am not optimistic about Gerrit from past experience. It does help gate
> > checkins and enforce pre-commit checks (good). I did not find it
> > user-friendly and it will be an additional hurdle for contributors to
> > understand (bad).
> >
> > Andrew, can the community build on your distributed pre-commit work to
> make
> > it production ready?
> >
> > Regards,
> > Arpit
> >
> >
> > On Mon, Jan 26, 2015 at 11:55 AM, Andrew Wang <an...@cloudera.com>
> > wrote:
> >
> > > Let's move this over to common-dev@, general@ is normally used for
> > project
> > > announcements rather than discussion topics.
> > >
> > > I'd like to summarize a few things mentioned on the private@ thread,
> > > related to streamlining the code submission process.
> > >
> > > - Gerrit was brought up again, as it has in the past, as something that
> > > could make the actual process of reviewing and committing a lot easier.
> > > This would be especially helpful for small patches, where the mechanics
> > of
> > > committing can take longer than reviewing the patch.
> > > - There were also concerns about forking discussions between JIRA and
> > > Gerrit. This has been an issue in Spark, and we'd like to keep
> > discussions
> > > and issue tracking centralized.
> > >
> > > - Some talk about how to improve precommit. Right now it takes hours to
> > run
> > > the unit tests, which slows down patch iterations. One solution is
> > running
> > > tests in parallel (and even distributed). Previous distributed
> > experiments
> > > have done a full unit test run in a couple minutes, but it'd be a fair
> > > amount of work to actually make this production ready.
> > > - Also mention of putting in place more linting and static analysis.
> > > Automating this will save reviewer time.
> > >
> > > Best,
> > > Andrew
> > >
> > > On Mon, Jan 26, 2015 at 9:16 AM, Ted Yu <yu...@gmail.com> wrote:
> > >
> > > > In some cases, contributor responded to review comments and attached
> > > > patches addressing the comments.
> > > >
> > > > Later on, there was simply no response to the latest patch - even
> with
> > > > follow-on ping.
> > > >
> > > > I wish this aspect can be improved.
> > > >
> > > > Cheers
> > > >
> > > > On Sun, Jan 25, 2015 at 6:03 PM, Tsz Wo (Nicholas), Sze <
> > > > s29752-hadoopgeneral@yahoo.com.invalid> wrote:
> > > >
> > > > > Hi contributors,
> > > > > I would like to (re)start a discussion regrading to our patch
> review
> > > > > process. A similar discussion has been happened in a the hadoop
> > > private
> > > > > mailing list, which is inappropriate.
> > > > > Here is the problem:The patch available queues become longer and
> > > longer.
> > > > > It seems that we never can catch up. There are patches sitting in
> > the
> > > > > queues for years. How could we speed up?
> > > > > Regrads,Tsz-Wo
> > > > >
> > > >
> > >
> >
> > --
> > CONFIDENTIALITY NOTICE
> > NOTICE: This message is intended for the use of the individual or entity
> to
> > which it is addressed and may contain information that is confidential,
> > privileged and exempt from disclosure under applicable law. If the reader
> > of this message is not the intended recipient, you are hereby notified
> that
> > any printing, copying, dissemination, distribution, disclosure or
> > forwarding of this communication is strictly prohibited. If you have
> > received this communication in error, please contact the sender
> immediately
> > and delete it from your system. Thank You.
> >
>
> --
> CONFIDENTIALITY NOTICE
> NOTICE: This message is intended for the use of the individual or entity to
> which it is addressed and may contain information that is confidential,
> privileged and exempt from disclosure under applicable law. If the reader
> of this message is not the intended recipient, you are hereby notified that
> any printing, copying, dissemination, distribution, disclosure or
> forwarding of this communication is strictly prohibited. If you have
> received this communication in error, please contact the sender immediately
> and delete it from your system. Thank You.
>
--
CONFIDENTIALITY NOTICE
NOTICE: This message is intended for the use of the individual or entity to
which it is addressed and may contain information that is confidential,
privileged and exempt from disclosure under applicable law. If the reader
of this message is not the intended recipient, you are hereby notified that
any printing, copying, dissemination, distribution, disclosure or
forwarding of this communication is strictly prohibited. If you have
received this communication in error, please contact the sender immediately
and delete it from your system. Thank You.
Re: Patch review process
Posted by Chris Nauroth <cn...@hortonworks.com>.
reviews.apache.org is backed by Review Board, and I've had a very positive
experience with that tool on other projects. HADOOP-9629 is a Hadoop patch
where we decided to go ahead and use it, and I think it helped. AFAIK,
there is no rule against using it in Hadoop, but it does have the side
effect of splitting part of the conversation out of jira. If Crucible can
keep all the review notes sync'd with the jira and searchable, then that
would be very interesting.
Chris Nauroth
Hortonworks
http://hortonworks.com/
On Mon, Jan 26, 2015 at 1:54 PM, Arpit Agarwal <aa...@hortonworks.com>
wrote:
> IMO the number one improvement would be a web-based review tool. We could
> evaluate Atlassian Crucible since it claims to integrate well with Jira. I
> have not tried https://reviews.apache.org/r/new/.
>
> Some easy improvements that were also raised by others on the private list:
> - Encourage contributors to batch related trivial fixes into a single
> patch.
> - Require more detailed descriptions with non-trivial patch contributions.
> For patches that require knowledge of a specific subsystem a
> background+design note would be a good start.
> - Eliminate CHANGES.txt. This came up on the dev list not too long ago and
> IIRC Allen did a PoC.
>
> I am not optimistic about Gerrit from past experience. It does help gate
> checkins and enforce pre-commit checks (good). I did not find it
> user-friendly and it will be an additional hurdle for contributors to
> understand (bad).
>
> Andrew, can the community build on your distributed pre-commit work to make
> it production ready?
>
> Regards,
> Arpit
>
>
> On Mon, Jan 26, 2015 at 11:55 AM, Andrew Wang <an...@cloudera.com>
> wrote:
>
> > Let's move this over to common-dev@, general@ is normally used for
> project
> > announcements rather than discussion topics.
> >
> > I'd like to summarize a few things mentioned on the private@ thread,
> > related to streamlining the code submission process.
> >
> > - Gerrit was brought up again, as it has in the past, as something that
> > could make the actual process of reviewing and committing a lot easier.
> > This would be especially helpful for small patches, where the mechanics
> of
> > committing can take longer than reviewing the patch.
> > - There were also concerns about forking discussions between JIRA and
> > Gerrit. This has been an issue in Spark, and we'd like to keep
> discussions
> > and issue tracking centralized.
> >
> > - Some talk about how to improve precommit. Right now it takes hours to
> run
> > the unit tests, which slows down patch iterations. One solution is
> running
> > tests in parallel (and even distributed). Previous distributed
> experiments
> > have done a full unit test run in a couple minutes, but it'd be a fair
> > amount of work to actually make this production ready.
> > - Also mention of putting in place more linting and static analysis.
> > Automating this will save reviewer time.
> >
> > Best,
> > Andrew
> >
> > On Mon, Jan 26, 2015 at 9:16 AM, Ted Yu <yu...@gmail.com> wrote:
> >
> > > In some cases, contributor responded to review comments and attached
> > > patches addressing the comments.
> > >
> > > Later on, there was simply no response to the latest patch - even with
> > > follow-on ping.
> > >
> > > I wish this aspect can be improved.
> > >
> > > Cheers
> > >
> > > On Sun, Jan 25, 2015 at 6:03 PM, Tsz Wo (Nicholas), Sze <
> > > s29752-hadoopgeneral@yahoo.com.invalid> wrote:
> > >
> > > > Hi contributors,
> > > > I would like to (re)start a discussion regrading to our patch review
> > > > process. A similar discussion has been happened in a the hadoop
> > private
> > > > mailing list, which is inappropriate.
> > > > Here is the problem:The patch available queues become longer and
> > longer.
> > > > It seems that we never can catch up. There are patches sitting in
> the
> > > > queues for years. How could we speed up?
> > > > Regrads,Tsz-Wo
> > > >
> > >
> >
>
> --
> CONFIDENTIALITY NOTICE
> NOTICE: This message is intended for the use of the individual or entity to
> which it is addressed and may contain information that is confidential,
> privileged and exempt from disclosure under applicable law. If the reader
> of this message is not the intended recipient, you are hereby notified that
> any printing, copying, dissemination, distribution, disclosure or
> forwarding of this communication is strictly prohibited. If you have
> received this communication in error, please contact the sender immediately
> and delete it from your system. Thank You.
>
--
CONFIDENTIALITY NOTICE
NOTICE: This message is intended for the use of the individual or entity to
which it is addressed and may contain information that is confidential,
privileged and exempt from disclosure under applicable law. If the reader
of this message is not the intended recipient, you are hereby notified that
any printing, copying, dissemination, distribution, disclosure or
forwarding of this communication is strictly prohibited. If you have
received this communication in error, please contact the sender immediately
and delete it from your system. Thank You.
Re: Patch review process
Posted by Andrew Wang <an...@cloudera.com>.
>
> Andrew, can the community build on your distributed pre-commit work to make
> it production ready?
>
>
I'm happy to share it if someone is willing to take it across the finish
line. I think it'd be about two weeks of work full-time. I've already
cleaned it up some, but it still requires checking out a couple forked
repos, and it's strung together with a set of bash scripts.
I see the biggest challenges as the following though:
- Need to write a faster Swarming client. Running the unit tests just takes
5 mins, but using the Python client to package up all the dependencies
takes 15-20 minutes.
- It needs to be deployed on Apache infra, integrated with Jenkins JUnit
reporting / artifact archiving, and then hooked up to the precommit
scripts. I also worry about the ongoing maintenance cost. Brock set up
something like this for Hive, and he had a hard time with flaky EC2
instances.
One additional upside though is this same framework should work for other
mavenized projects too. There's nothing Hadoop specific in it right now, so
HBase, Solr, etc could leverage it too.
Best,
Andrew
Re: Patch review process
Posted by Arpit Agarwal <aa...@hortonworks.com>.
IMO the number one improvement would be a web-based review tool. We could
evaluate Atlassian Crucible since it claims to integrate well with Jira. I
have not tried https://reviews.apache.org/r/new/.
Some easy improvements that were also raised by others on the private list:
- Encourage contributors to batch related trivial fixes into a single patch.
- Require more detailed descriptions with non-trivial patch contributions.
For patches that require knowledge of a specific subsystem a
background+design note would be a good start.
- Eliminate CHANGES.txt. This came up on the dev list not too long ago and
IIRC Allen did a PoC.
I am not optimistic about Gerrit from past experience. It does help gate
checkins and enforce pre-commit checks (good). I did not find it
user-friendly and it will be an additional hurdle for contributors to
understand (bad).
Andrew, can the community build on your distributed pre-commit work to make
it production ready?
Regards,
Arpit
On Mon, Jan 26, 2015 at 11:55 AM, Andrew Wang <an...@cloudera.com>
wrote:
> Let's move this over to common-dev@, general@ is normally used for project
> announcements rather than discussion topics.
>
> I'd like to summarize a few things mentioned on the private@ thread,
> related to streamlining the code submission process.
>
> - Gerrit was brought up again, as it has in the past, as something that
> could make the actual process of reviewing and committing a lot easier.
> This would be especially helpful for small patches, where the mechanics of
> committing can take longer than reviewing the patch.
> - There were also concerns about forking discussions between JIRA and
> Gerrit. This has been an issue in Spark, and we'd like to keep discussions
> and issue tracking centralized.
>
> - Some talk about how to improve precommit. Right now it takes hours to run
> the unit tests, which slows down patch iterations. One solution is running
> tests in parallel (and even distributed). Previous distributed experiments
> have done a full unit test run in a couple minutes, but it'd be a fair
> amount of work to actually make this production ready.
> - Also mention of putting in place more linting and static analysis.
> Automating this will save reviewer time.
>
> Best,
> Andrew
>
> On Mon, Jan 26, 2015 at 9:16 AM, Ted Yu <yu...@gmail.com> wrote:
>
> > In some cases, contributor responded to review comments and attached
> > patches addressing the comments.
> >
> > Later on, there was simply no response to the latest patch - even with
> > follow-on ping.
> >
> > I wish this aspect can be improved.
> >
> > Cheers
> >
> > On Sun, Jan 25, 2015 at 6:03 PM, Tsz Wo (Nicholas), Sze <
> > s29752-hadoopgeneral@yahoo.com.invalid> wrote:
> >
> > > Hi contributors,
> > > I would like to (re)start a discussion regrading to our patch review
> > > process. A similar discussion has been happened in a the hadoop
> private
> > > mailing list, which is inappropriate.
> > > Here is the problem:The patch available queues become longer and
> longer.
> > > It seems that we never can catch up. There are patches sitting in the
> > > queues for years. How could we speed up?
> > > Regrads,Tsz-Wo
> > >
> >
>
--
CONFIDENTIALITY NOTICE
NOTICE: This message is intended for the use of the individual or entity to
which it is addressed and may contain information that is confidential,
privileged and exempt from disclosure under applicable law. If the reader
of this message is not the intended recipient, you are hereby notified that
any printing, copying, dissemination, distribution, disclosure or
forwarding of this communication is strictly prohibited. If you have
received this communication in error, please contact the sender immediately
and delete it from your system. Thank You.
Re: Patch review process
Posted by Steve Loughran <st...@hortonworks.com>.
I'm moving this all to common-dev@ as general is more where announcements
go than anything else.
I think too many patches are falling by the wayside. It takes a lot of time
and effort to get patches in, small patches that aren't viewed as critical
tend to atrophy: lost in "patch-availalble" state, nobody tracking their
status, just someone left feeling neglected. Yet many of those patches help
the codebase: fix minor issues, improve the readability of the code, keep
that code in sync with changing dependencies (e.g. an evolving guava). Just
chasing down @Deprecated warnings would be a start.
To a committer, those small patches are work: there's still the overhead of
looking at them, and the manual effort of the merge which is constant for
all patches (update/reset your branch, apply, test, CHANGES.TXT, commit,
cherry-pick, push). Unless you view them as critical its just too much
effort for the minor stuff.
And as a developer of the small things, you get the other side of the work:
having to rebase and track trunk, resubmit, wait, maybe get some feedback,
otherwise: silence.
Even I have lots of minor patches being neglected, looking at those I see
one, HADOOP-6221 (*) Make it possible to interrupt RPC Client operations,
dates from 2009. That's five years old, impacts production —and still
nobody looks at it. And if a committer can't get patches in after 5 years,
what chance do others have?
Here are some thoughts of mine
*Technology*
-could something like Gerrit help?
-could we do more with Git pull requests? One issue here is that none of us
want the completely history of a patch development merged in, only an
aggregate (squashed) patch.
-How can testing be improved? That's precommit and postcommit regression
testing
*Codebase*
Culling hadoop-contrib was a good thing: it was where code went to die. But
we now have things that are considered to matter, yet under-reviewed.
Object stores are a key example: nobody full time works on those,
especially s3 and openstack —and everyone forgts that jenkins doesn't run
their tests (which add time and cost money). Which is how a release of
hadoop got out (2.4) whose S3n client would NPE on a seek(0) of a 0-byte
file. What bits like this are considered to matter, yet don't get much
attention?
Off-hand, I'd say: launcher scripts & metrics; There's the perennial
'update the dependency' work (HADOOP-9991) which is painful, and somewhat
risky. Which is why we tend to stick with dependencies we know, even if we
know they are outdated and have problems (jetty is many generations out of
date, for example)
*Process*
-How can we clear the backlog?
-How do we keep it lean in future?
-what's the best way of developing "housekeeping" patches, ones that apply
the same fixes everywhere (e.g moving off some deprecated Guava API). These
patches age fast, yet nobody is going to want to review & process the
changes one file at a time.
So: discuss away.
In the meantime, I am planning to review and where possible commit patches
from people who are not committers but have submissions which matter to
them, with the goal being to +1 in stuff that has been out there a
while.(**)
-Steve
(*) https://issues.apache.org/jira/browse/HADOOP-6221 did get in +1'd
earlier today, but there are many more in that state, even just from me.
(**) If you have had >1 thing committed recently please don't ask for more,
I am trying to get more people's work in.
On 26 January 2015 at 19:55, Andrew Wang <an...@cloudera.com> wrote:
> Let's move this over to common-dev@, general@ is normally used for project
> announcements rather than discussion topics.
>
> I'd like to summarize a few things mentioned on the private@ thread,
> related to streamlining the code submission process.
>
> - Gerrit was brought up again, as it has in the past, as something that
> could make the actual process of reviewing and committing a lot easier.
> This would be especially helpful for small patches, where the mechanics of
> committing can take longer than reviewing the patch.
> - There were also concerns about forking discussions between JIRA and
> Gerrit. This has been an issue in Spark, and we'd like to keep discussions
> and issue tracking centralized.
>
> - Some talk about how to improve precommit. Right now it takes hours to run
> the unit tests, which slows down patch iterations. One solution is running
> tests in parallel (and even distributed). Previous distributed experiments
> have done a full unit test run in a couple minutes, but it'd be a fair
> amount of work to actually make this production ready.
> - Also mention of putting in place more linting and static analysis.
> Automating this will save reviewer time.
>
> Best,
> Andrew
>
> On Mon, Jan 26, 2015 at 9:16 AM, Ted Yu <yu...@gmail.com> wrote:
>
> > In some cases, contributor responded to review comments and attached
> > patches addressing the comments.
> >
> > Later on, there was simply no response to the latest patch - even with
> > follow-on ping.
> >
> > I wish this aspect can be improved.
> >
> > Cheers
> >
> > On Sun, Jan 25, 2015 at 6:03 PM, Tsz Wo (Nicholas), Sze <
> > s29752-hadoopgeneral@yahoo.com.invalid> wrote:
> >
> > > Hi contributors,
> > > I would like to (re)start a discussion regrading to our patch review
> > > process. A similar discussion has been happened in a the hadoop
> private
> > > mailing list, which is inappropriate.
> > > Here is the problem:The patch available queues become longer and
> longer.
> > > It seems that we never can catch up. There are patches sitting in the
> > > queues for years. How could we speed up?
> > > Regrads,Tsz-Wo
> > >
> >
>
--
CONFIDENTIALITY NOTICE
NOTICE: This message is intended for the use of the individual or entity to
which it is addressed and may contain information that is confidential,
privileged and exempt from disclosure under applicable law. If the reader
of this message is not the intended recipient, you are hereby notified that
any printing, copying, dissemination, distribution, disclosure or
forwarding of this communication is strictly prohibited. If you have
received this communication in error, please contact the sender immediately
and delete it from your system. Thank You.