You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Pid <pi...@pidster.com> on 2015/05/07 11:43:19 UTC
[DISCUSS] Review process + Git Flow
Hi,
Like it says, can we discuss how the review process will work?
For these examples:
1. I would like to work on upgrading the Spring dependencies in gfsh.
Proposed approach: file a JIRA, cut a feature branch, push it & then what?
2. I would like to add an entry to .gitignore (.idea/)
Does this require a JIRA, a feature branch and a review?
p
--
[key:62590808]
Re: [DISCUSS] Review process + Git Flow
Posted by Anthony Baker <ab...@pivotal.io>.
Good points jan. I think many OSS communities don’t go as far as Spark did in codifying component leadership. Do you find that reviewers tend to self-select around code areas they have expertise in?
Anthony
> On May 7, 2015, at 10:50 AM, jan i <ja...@apache.org> wrote:
>
> ý
>
> On Thursday, May 7, 2015, William Markito <wmarkito@pivotal.io <ma...@pivotal.io>> wrote:
>
>> On Thu, May 7, 2015 at 10:04 AM, Pid <pid@pidster.com <javascript:;>>
>> wrote:
>>
>>> On 07/05/2015 15:13, Anthony Baker wrote:
>>>> Agree that RTC is really important. In addition, we should consider
>>> that some changes require specific knowledge and context (I’m thinking of
>>> you AbstractRegionMap). Note that I’m not advocating for code ownership.
>>> Spark [1] uses this approach:
>>>>
>>>> "For certain modules, changes to the architecture and public API should
>>> also be reviewed by a maintainer for that module (which may or may not be
>>> the same as the main reviewer) before being merged. The PMC has
>> designated
>>> the following maintainers…”
>>>>
>>>> Changes to public API’s or core internals would fall into this
>>> category. Thoughts?
>>>
>>> Good points on context; there's a lot of code to understand.
>>>
>>> Is knowledge about specific components concentrated in specific people
>>> (or groups of people) in the existing developer team?
>>>
>>>
>> Things will get more clear when have the components under Jira. We should
>> then have the leaders (experts) for such components and they will have to
>> review such critical changes.
>
>
> Having leaders for components that must review changes is very hard to
> combine with an open community. It is ok and a good idea to have
> specialists and take their advice seriously.
>
> In a community everybody should be equal and have the same rights.
>
> just my opinion.
> rgds
> jan i
>
>
>>
>>
>>
>>>
>>>
>>>
>>>> Anthony
>>>>
>>>> [1] https://cwiki.apache.org/confluence/display/SPARK/Committers
>>>>
>>>>
>>>>
>>>>> On May 7, 2015, at 3:38 AM, Justin Erenkrantz <justin@erenkrantz.com
>> <javascript:;>>
>>> wrote:
>>>>>
>>>>> One question that we need to discuss is whether every merge is RTC
>>>>> (Review-than-Commit) or CTR (Commit-than-Review).
>>>>>
>>>>> My take is that we should start with RTC and, if the review process
>>> gets in
>>>>> the way of innovation, then we go to CTR. But, until everyone learns
>>> the
>>>>> rules of the road, I think RTC is justified. Under RTC rules, all
>>> commits
>>>>> should be reviewed (+1) by three committers before being merged. (If
>>> you
>>>>> are a committer, then two others are needed.). Any committer can veto
>>> (-1)
>>>>> a patch - which should cause a discussion about resolving the veto.
>>>>>
>>>>> So, #1 - your suggestion sounds right with the need for three
>>> committers to
>>>>> approve before merge to develop.
>>>>>
>>>>> For #2, I think it should be a separate branch and require 3 signoffs
>>> for
>>>>> now.
>>>>>
>>>>> As the project matures, "obvious" commits can be CTR.
>>>>>
>>>>> My $.02. -- justin
>>>>> On May 7, 2015 5:44 AM, "Pid" <pid@pidster.com <javascript:;>> wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>>
>>>>>> Like it says, can we discuss how the review process will work?
>>>>>> For these examples:
>>>>>>
>>>>>>
>>>>>> 1. I would like to work on upgrading the Spring dependencies in gfsh.
>>>>>>
>>>>>> Proposed approach: file a JIRA, cut a feature branch, push it & then
>>> what?
>>>>>>
>>>>>>
>>>>>> 2. I would like to add an entry to .gitignore (.idea/)
>>>>>>
>>>>>> Does this require a JIRA, a feature branch and a review?
>>>>>>
>>>>>>
>>>>>>
>>>>>> p
>>>>>>
>>>>>> --
>>>>>>
>>>>>> [key:62590808]
>>>>>>
>>>>
>>>
>>>
>>> --
>>>
>>> [key:62590808]
>>>
>>
>>
>>
>> --
>>
>> William Markito Oliveira
>> Enterprise Architect
>> *http://www.pivotal.io/ <http://www.pivotal.io/>*
>> For prompt responses to questions on GemFire/GemFireXD, please write
>> to *rtds-dev-ea
>> at pivotal.io <http://pivotal.io>*
>>
>
>
> --
> Sent from My iPad, sorry for any misspellings.
Re: [DISCUSS] Review process + Git Flow
Posted by jan i <ja...@apache.org>.
ý
On Thursday, May 7, 2015, William Markito <wm...@pivotal.io> wrote:
> On Thu, May 7, 2015 at 10:04 AM, Pid <pid@pidster.com <javascript:;>>
> wrote:
>
> > On 07/05/2015 15:13, Anthony Baker wrote:
> > > Agree that RTC is really important. In addition, we should consider
> > that some changes require specific knowledge and context (I’m thinking of
> > you AbstractRegionMap). Note that I’m not advocating for code ownership.
> > Spark [1] uses this approach:
> > >
> > > "For certain modules, changes to the architecture and public API should
> > also be reviewed by a maintainer for that module (which may or may not be
> > the same as the main reviewer) before being merged. The PMC has
> designated
> > the following maintainers…”
> > >
> > > Changes to public API’s or core internals would fall into this
> > category. Thoughts?
> >
> > Good points on context; there's a lot of code to understand.
> >
> > Is knowledge about specific components concentrated in specific people
> > (or groups of people) in the existing developer team?
> >
> >
> Things will get more clear when have the components under Jira. We should
> then have the leaders (experts) for such components and they will have to
> review such critical changes.
Having leaders for components that must review changes is very hard to
combine with an open community. It is ok and a good idea to have
specialists and take their advice seriously.
In a community everybody should be equal and have the same rights.
just my opinion.
rgds
jan i
>
>
>
> >
> >
> >
> > > Anthony
> > >
> > > [1] https://cwiki.apache.org/confluence/display/SPARK/Committers
> > >
> > >
> > >
> > >> On May 7, 2015, at 3:38 AM, Justin Erenkrantz <justin@erenkrantz.com
> <javascript:;>>
> > wrote:
> > >>
> > >> One question that we need to discuss is whether every merge is RTC
> > >> (Review-than-Commit) or CTR (Commit-than-Review).
> > >>
> > >> My take is that we should start with RTC and, if the review process
> > gets in
> > >> the way of innovation, then we go to CTR. But, until everyone learns
> > the
> > >> rules of the road, I think RTC is justified. Under RTC rules, all
> > commits
> > >> should be reviewed (+1) by three committers before being merged. (If
> > you
> > >> are a committer, then two others are needed.). Any committer can veto
> > (-1)
> > >> a patch - which should cause a discussion about resolving the veto.
> > >>
> > >> So, #1 - your suggestion sounds right with the need for three
> > committers to
> > >> approve before merge to develop.
> > >>
> > >> For #2, I think it should be a separate branch and require 3 signoffs
> > for
> > >> now.
> > >>
> > >> As the project matures, "obvious" commits can be CTR.
> > >>
> > >> My $.02. -- justin
> > >> On May 7, 2015 5:44 AM, "Pid" <pid@pidster.com <javascript:;>> wrote:
> > >>
> > >>> Hi,
> > >>>
> > >>>
> > >>> Like it says, can we discuss how the review process will work?
> > >>> For these examples:
> > >>>
> > >>>
> > >>> 1. I would like to work on upgrading the Spring dependencies in gfsh.
> > >>>
> > >>> Proposed approach: file a JIRA, cut a feature branch, push it & then
> > what?
> > >>>
> > >>>
> > >>> 2. I would like to add an entry to .gitignore (.idea/)
> > >>>
> > >>> Does this require a JIRA, a feature branch and a review?
> > >>>
> > >>>
> > >>>
> > >>> p
> > >>>
> > >>> --
> > >>>
> > >>> [key:62590808]
> > >>>
> > >
> >
> >
> > --
> >
> > [key:62590808]
> >
>
>
>
> --
>
> William Markito Oliveira
> Enterprise Architect
> *http://www.pivotal.io/ <http://www.pivotal.io/>*
> For prompt responses to questions on GemFire/GemFireXD, please write
> to *rtds-dev-ea
> at pivotal.io <http://pivotal.io>*
>
--
Sent from My iPad, sorry for any misspellings.
Re: [DISCUSS] Review process + Git Flow
Posted by William Markito <wm...@pivotal.io>.
On Thu, May 7, 2015 at 10:04 AM, Pid <pi...@pidster.com> wrote:
> On 07/05/2015 15:13, Anthony Baker wrote:
> > Agree that RTC is really important. In addition, we should consider
> that some changes require specific knowledge and context (I’m thinking of
> you AbstractRegionMap). Note that I’m not advocating for code ownership.
> Spark [1] uses this approach:
> >
> > "For certain modules, changes to the architecture and public API should
> also be reviewed by a maintainer for that module (which may or may not be
> the same as the main reviewer) before being merged. The PMC has designated
> the following maintainers…”
> >
> > Changes to public API’s or core internals would fall into this
> category. Thoughts?
>
> Good points on context; there's a lot of code to understand.
>
> Is knowledge about specific components concentrated in specific people
> (or groups of people) in the existing developer team?
>
>
Things will get more clear when have the components under Jira. We should
then have the leaders (experts) for such components and they will have to
review such critical changes.
>
>
>
> > Anthony
> >
> > [1] https://cwiki.apache.org/confluence/display/SPARK/Committers
> >
> >
> >
> >> On May 7, 2015, at 3:38 AM, Justin Erenkrantz <ju...@erenkrantz.com>
> wrote:
> >>
> >> One question that we need to discuss is whether every merge is RTC
> >> (Review-than-Commit) or CTR (Commit-than-Review).
> >>
> >> My take is that we should start with RTC and, if the review process
> gets in
> >> the way of innovation, then we go to CTR. But, until everyone learns
> the
> >> rules of the road, I think RTC is justified. Under RTC rules, all
> commits
> >> should be reviewed (+1) by three committers before being merged. (If
> you
> >> are a committer, then two others are needed.). Any committer can veto
> (-1)
> >> a patch - which should cause a discussion about resolving the veto.
> >>
> >> So, #1 - your suggestion sounds right with the need for three
> committers to
> >> approve before merge to develop.
> >>
> >> For #2, I think it should be a separate branch and require 3 signoffs
> for
> >> now.
> >>
> >> As the project matures, "obvious" commits can be CTR.
> >>
> >> My $.02. -- justin
> >> On May 7, 2015 5:44 AM, "Pid" <pi...@pidster.com> wrote:
> >>
> >>> Hi,
> >>>
> >>>
> >>> Like it says, can we discuss how the review process will work?
> >>> For these examples:
> >>>
> >>>
> >>> 1. I would like to work on upgrading the Spring dependencies in gfsh.
> >>>
> >>> Proposed approach: file a JIRA, cut a feature branch, push it & then
> what?
> >>>
> >>>
> >>> 2. I would like to add an entry to .gitignore (.idea/)
> >>>
> >>> Does this require a JIRA, a feature branch and a review?
> >>>
> >>>
> >>>
> >>> p
> >>>
> >>> --
> >>>
> >>> [key:62590808]
> >>>
> >
>
>
> --
>
> [key:62590808]
>
--
William Markito Oliveira
Enterprise Architect
*http://www.pivotal.io/ <http://www.pivotal.io/>*
For prompt responses to questions on GemFire/GemFireXD, please write
to *rtds-dev-ea
at pivotal.io <http://pivotal.io>*
Re: [DISCUSS] Review process + Git Flow
Posted by Pid <pi...@pidster.com>.
On 07/05/2015 15:13, Anthony Baker wrote:
> Agree that RTC is really important. In addition, we should consider that some changes require specific knowledge and context (I’m thinking of you AbstractRegionMap). Note that I’m not advocating for code ownership. Spark [1] uses this approach:
>
> "For certain modules, changes to the architecture and public API should also be reviewed by a maintainer for that module (which may or may not be the same as the main reviewer) before being merged. The PMC has designated the following maintainers…”
>
> Changes to public API’s or core internals would fall into this category. Thoughts?
Good points on context; there's a lot of code to understand.
Is knowledge about specific components concentrated in specific people
(or groups of people) in the existing developer team?
> Anthony
>
> [1] https://cwiki.apache.org/confluence/display/SPARK/Committers
>
>
>
>> On May 7, 2015, at 3:38 AM, Justin Erenkrantz <ju...@erenkrantz.com> wrote:
>>
>> One question that we need to discuss is whether every merge is RTC
>> (Review-than-Commit) or CTR (Commit-than-Review).
>>
>> My take is that we should start with RTC and, if the review process gets in
>> the way of innovation, then we go to CTR. But, until everyone learns the
>> rules of the road, I think RTC is justified. Under RTC rules, all commits
>> should be reviewed (+1) by three committers before being merged. (If you
>> are a committer, then two others are needed.). Any committer can veto (-1)
>> a patch - which should cause a discussion about resolving the veto.
>>
>> So, #1 - your suggestion sounds right with the need for three committers to
>> approve before merge to develop.
>>
>> For #2, I think it should be a separate branch and require 3 signoffs for
>> now.
>>
>> As the project matures, "obvious" commits can be CTR.
>>
>> My $.02. -- justin
>> On May 7, 2015 5:44 AM, "Pid" <pi...@pidster.com> wrote:
>>
>>> Hi,
>>>
>>>
>>> Like it says, can we discuss how the review process will work?
>>> For these examples:
>>>
>>>
>>> 1. I would like to work on upgrading the Spring dependencies in gfsh.
>>>
>>> Proposed approach: file a JIRA, cut a feature branch, push it & then what?
>>>
>>>
>>> 2. I would like to add an entry to .gitignore (.idea/)
>>>
>>> Does this require a JIRA, a feature branch and a review?
>>>
>>>
>>>
>>> p
>>>
>>> --
>>>
>>> [key:62590808]
>>>
>
--
[key:62590808]
Re: [DISCUSS] Review process + Git Flow
Posted by William Markito <wm...@pivotal.io>.
+1
On Thu, May 7, 2015 at 3:36 PM, Konstantin Boudnik <co...@apache.org> wrote:
> On Thu, May 07, 2015 at 02:54PM, William Markito wrote:
> > Just to complement what I have said about component leaders:
> >
> > The important thing about this kind of assignment wouldn't be to have
> only
> > a single person to control them, but much more on to help the community
> to
> > work with the experts of each area/component and also to assign some
> > responsibility to this people to review/look at the changes.
> >
> > We can always argue both ways, RTC or CTR have their own merits and
> > benefits, but in the end of the day what we probably want is to make sure
> > that the code base is stable and the core principles of the project
> > (performance, for one) are always taken care of.
>
> Indeed. Although, the more important role of the mentors is to make sure
> that
> we are building a sound and vibrant community, which lives to "community
> over
> the code" (a somewhat counter-intuitive Apache) motto
>
> Cos
>
> > On Thu, May 7, 2015 at 2:19 PM, Konstantin Boudnik <co...@apache.org>
> wrote:
> >
> > > RTC is great, but it tends to slow down the contribution process (for
> > > existing
> > > committers) and also might (in extreme cases) lead to a low-quality
> > > reviews,
> > > as it adds more work on developers. CTR on the other hand, will let the
> > > changes to go in much faster but it requires a few things:
> > > - mature committers who won't commit crap into the common code base
> > > - well-oiled CI to ensure sufficient testing of new commits
> > > - optionally, a longer time to gain the commit-bit
> > >
> > > Spark example is a bit extreme and I don't think it flies well with the
> > > board@ nor it is really compatible with open-community model.
> > > There's nothing wrong in having maintainers (for once we have it in
> > > Bigtop),
> > > but it is certainly wrong to promote these maintainers into the status
> of a
> > > component kings.
> > >
> > > Regards,
> > > Cos
> > >
> > > On Thu, May 07, 2015 at 07:13AM, Anthony Baker wrote:
> > > > Agree that RTC is really important. In addition, we should consider
> that
> > > > some changes require specific knowledge and context (I’m thinking of
> you
> > > > AbstractRegionMap). Note that I’m not advocating for code ownership.
> > > Spark
> > > > [1] uses this approach:
> > > >
> > > > "For certain modules, changes to the architecture and public API
> should
> > > also
> > > > be reviewed by a maintainer for that module (which may or may not be
> the
> > > > same as the main reviewer) before being merged. The PMC has
> designated
> > > the
> > > > following maintainers…”
> > > >
> > > > Changes to public API’s or core internals would fall into this
> > > category. Thoughts?
> > > >
> > > >
> > > > Anthony
> > > >
> > > > [1] https://cwiki.apache.org/confluence/display/SPARK/Committers
> > > >
> > > >
> > > >
> > > > > On May 7, 2015, at 3:38 AM, Justin Erenkrantz <
> justin@erenkrantz.com>
> > > wrote:
> > > > >
> > > > > One question that we need to discuss is whether every merge is RTC
> > > > > (Review-than-Commit) or CTR (Commit-than-Review).
> > > > >
> > > > > My take is that we should start with RTC and, if the review process
> > > gets in
> > > > > the way of innovation, then we go to CTR. But, until everyone
> learns
> > > the
> > > > > rules of the road, I think RTC is justified. Under RTC rules, all
> > > commits
> > > > > should be reviewed (+1) by three committers before being merged.
> (If
> > > you
> > > > > are a committer, then two others are needed.). Any committer can
> veto
> > > (-1)
> > > > > a patch - which should cause a discussion about resolving the veto.
> > > > >
> > > > > So, #1 - your suggestion sounds right with the need for three
> > > committers to
> > > > > approve before merge to develop.
> > > > >
> > > > > For #2, I think it should be a separate branch and require 3
> signoffs
> > > for
> > > > > now.
> > > > >
> > > > > As the project matures, "obvious" commits can be CTR.
> > > > >
> > > > > My $.02. -- justin
> > > > > On May 7, 2015 5:44 AM, "Pid" <pi...@pidster.com> wrote:
> > > > >
> > > > >> Hi,
> > > > >>
> > > > >>
> > > > >> Like it says, can we discuss how the review process will work?
> > > > >> For these examples:
> > > > >>
> > > > >>
> > > > >> 1. I would like to work on upgrading the Spring dependencies in
> gfsh.
> > > > >>
> > > > >> Proposed approach: file a JIRA, cut a feature branch, push it &
> then
> > > what?
> > > > >>
> > > > >>
> > > > >> 2. I would like to add an entry to .gitignore (.idea/)
> > > > >>
> > > > >> Does this require a JIRA, a feature branch and a review?
> > > > >>
> > > > >>
> > > > >>
> > > > >> p
> > > > >>
> > > > >> --
> > > > >>
> > > > >> [key:62590808]
> > > > >>
> > > >
> > >
> >
> >
> >
> > --
> >
> > William Markito Oliveira
> > Enterprise Architect
> > *http://www.pivotal.io/ <http://www.pivotal.io/>*
> > For prompt responses to questions on GemFire/GemFireXD, please write
> > to *rtds-dev-ea
> > at pivotal.io <http://pivotal.io>*
>
--
William Markito Oliveira
Enterprise Architect
*http://www.pivotal.io/ <http://www.pivotal.io/>*
For prompt responses to questions on GemFire/GemFireXD, please write
to *rtds-dev-ea
at pivotal.io <http://pivotal.io>*
Re: [DISCUSS] Review process + Git Flow
Posted by Konstantin Boudnik <co...@apache.org>.
On Thu, May 07, 2015 at 02:54PM, William Markito wrote:
> Just to complement what I have said about component leaders:
>
> The important thing about this kind of assignment wouldn't be to have only
> a single person to control them, but much more on to help the community to
> work with the experts of each area/component and also to assign some
> responsibility to this people to review/look at the changes.
>
> We can always argue both ways, RTC or CTR have their own merits and
> benefits, but in the end of the day what we probably want is to make sure
> that the code base is stable and the core principles of the project
> (performance, for one) are always taken care of.
Indeed. Although, the more important role of the mentors is to make sure that
we are building a sound and vibrant community, which lives to "community over
the code" (a somewhat counter-intuitive Apache) motto
Cos
> On Thu, May 7, 2015 at 2:19 PM, Konstantin Boudnik <co...@apache.org> wrote:
>
> > RTC is great, but it tends to slow down the contribution process (for
> > existing
> > committers) and also might (in extreme cases) lead to a low-quality
> > reviews,
> > as it adds more work on developers. CTR on the other hand, will let the
> > changes to go in much faster but it requires a few things:
> > - mature committers who won't commit crap into the common code base
> > - well-oiled CI to ensure sufficient testing of new commits
> > - optionally, a longer time to gain the commit-bit
> >
> > Spark example is a bit extreme and I don't think it flies well with the
> > board@ nor it is really compatible with open-community model.
> > There's nothing wrong in having maintainers (for once we have it in
> > Bigtop),
> > but it is certainly wrong to promote these maintainers into the status of a
> > component kings.
> >
> > Regards,
> > Cos
> >
> > On Thu, May 07, 2015 at 07:13AM, Anthony Baker wrote:
> > > Agree that RTC is really important. In addition, we should consider that
> > > some changes require specific knowledge and context (I’m thinking of you
> > > AbstractRegionMap). Note that I’m not advocating for code ownership.
> > Spark
> > > [1] uses this approach:
> > >
> > > "For certain modules, changes to the architecture and public API should
> > also
> > > be reviewed by a maintainer for that module (which may or may not be the
> > > same as the main reviewer) before being merged. The PMC has designated
> > the
> > > following maintainers…”
> > >
> > > Changes to public API’s or core internals would fall into this
> > category. Thoughts?
> > >
> > >
> > > Anthony
> > >
> > > [1] https://cwiki.apache.org/confluence/display/SPARK/Committers
> > >
> > >
> > >
> > > > On May 7, 2015, at 3:38 AM, Justin Erenkrantz <ju...@erenkrantz.com>
> > wrote:
> > > >
> > > > One question that we need to discuss is whether every merge is RTC
> > > > (Review-than-Commit) or CTR (Commit-than-Review).
> > > >
> > > > My take is that we should start with RTC and, if the review process
> > gets in
> > > > the way of innovation, then we go to CTR. But, until everyone learns
> > the
> > > > rules of the road, I think RTC is justified. Under RTC rules, all
> > commits
> > > > should be reviewed (+1) by three committers before being merged. (If
> > you
> > > > are a committer, then two others are needed.). Any committer can veto
> > (-1)
> > > > a patch - which should cause a discussion about resolving the veto.
> > > >
> > > > So, #1 - your suggestion sounds right with the need for three
> > committers to
> > > > approve before merge to develop.
> > > >
> > > > For #2, I think it should be a separate branch and require 3 signoffs
> > for
> > > > now.
> > > >
> > > > As the project matures, "obvious" commits can be CTR.
> > > >
> > > > My $.02. -- justin
> > > > On May 7, 2015 5:44 AM, "Pid" <pi...@pidster.com> wrote:
> > > >
> > > >> Hi,
> > > >>
> > > >>
> > > >> Like it says, can we discuss how the review process will work?
> > > >> For these examples:
> > > >>
> > > >>
> > > >> 1. I would like to work on upgrading the Spring dependencies in gfsh.
> > > >>
> > > >> Proposed approach: file a JIRA, cut a feature branch, push it & then
> > what?
> > > >>
> > > >>
> > > >> 2. I would like to add an entry to .gitignore (.idea/)
> > > >>
> > > >> Does this require a JIRA, a feature branch and a review?
> > > >>
> > > >>
> > > >>
> > > >> p
> > > >>
> > > >> --
> > > >>
> > > >> [key:62590808]
> > > >>
> > >
> >
>
>
>
> --
>
> William Markito Oliveira
> Enterprise Architect
> *http://www.pivotal.io/ <http://www.pivotal.io/>*
> For prompt responses to questions on GemFire/GemFireXD, please write
> to *rtds-dev-ea
> at pivotal.io <http://pivotal.io>*
Re: [DISCUSS] Review process + Git Flow
Posted by Roman Shaposhnik <ro...@shaposhnik.org>.
On Thu, May 7, 2015 at 2:54 PM, William Markito <wm...@pivotal.io> wrote:
> Just to complement what I have said about component leaders:
>
> The important thing about this kind of assignment wouldn't be to have only
> a single person to control them, but much more on to help the community to
> work with the experts of each area/component and also to assign some
> responsibility to this people to review/look at the changes.
>
> We can always argue both ways, RTC or CTR have their own merits and
> benefits, but in the end of the day what we probably want is to make sure
> that the code base is stable and the core principles of the project
> (performance, for one) are always taken care of.
Well, at this stage what we really need is to make sure that we grow
the community. Now, of course, I'm not suggesting that we wreck the
stability of the project by doing that, but rather have mentors who can
onboard folks on the areas of their expertise. IOW, think of these 'owners'
as the first responders to issues that fall into the area of their expertise.
We should absolutely compile a list of such experts on our wiki and also
make them JIRA owners of particular areas of Geode.
The expectations, once again, will be that they will be the known people
to ping for feedback, etc not that they would be required to sign-off
on every single commit.
After all, we're using SCM for a reason: things can be backed out. And
if a community backs out your change it sends a pretty strong signal
that you may be better off asking for an expert opinion next time. The
good news is that we will maintain that list of experts for you.
Thanks,
Roman.
Re: [DISCUSS] Review process + Git Flow
Posted by William Markito <wm...@pivotal.io>.
Just to complement what I have said about component leaders:
The important thing about this kind of assignment wouldn't be to have only
a single person to control them, but much more on to help the community to
work with the experts of each area/component and also to assign some
responsibility to this people to review/look at the changes.
We can always argue both ways, RTC or CTR have their own merits and
benefits, but in the end of the day what we probably want is to make sure
that the code base is stable and the core principles of the project
(performance, for one) are always taken care of.
Just my .002c
On Thu, May 7, 2015 at 2:19 PM, Konstantin Boudnik <co...@apache.org> wrote:
> RTC is great, but it tends to slow down the contribution process (for
> existing
> committers) and also might (in extreme cases) lead to a low-quality
> reviews,
> as it adds more work on developers. CTR on the other hand, will let the
> changes to go in much faster but it requires a few things:
> - mature committers who won't commit crap into the common code base
> - well-oiled CI to ensure sufficient testing of new commits
> - optionally, a longer time to gain the commit-bit
>
> Spark example is a bit extreme and I don't think it flies well with the
> board@ nor it is really compatible with open-community model.
> There's nothing wrong in having maintainers (for once we have it in
> Bigtop),
> but it is certainly wrong to promote these maintainers into the status of a
> component kings.
>
> Regards,
> Cos
>
> On Thu, May 07, 2015 at 07:13AM, Anthony Baker wrote:
> > Agree that RTC is really important. In addition, we should consider that
> > some changes require specific knowledge and context (I’m thinking of you
> > AbstractRegionMap). Note that I’m not advocating for code ownership.
> Spark
> > [1] uses this approach:
> >
> > "For certain modules, changes to the architecture and public API should
> also
> > be reviewed by a maintainer for that module (which may or may not be the
> > same as the main reviewer) before being merged. The PMC has designated
> the
> > following maintainers…”
> >
> > Changes to public API’s or core internals would fall into this
> category. Thoughts?
> >
> >
> > Anthony
> >
> > [1] https://cwiki.apache.org/confluence/display/SPARK/Committers
> >
> >
> >
> > > On May 7, 2015, at 3:38 AM, Justin Erenkrantz <ju...@erenkrantz.com>
> wrote:
> > >
> > > One question that we need to discuss is whether every merge is RTC
> > > (Review-than-Commit) or CTR (Commit-than-Review).
> > >
> > > My take is that we should start with RTC and, if the review process
> gets in
> > > the way of innovation, then we go to CTR. But, until everyone learns
> the
> > > rules of the road, I think RTC is justified. Under RTC rules, all
> commits
> > > should be reviewed (+1) by three committers before being merged. (If
> you
> > > are a committer, then two others are needed.). Any committer can veto
> (-1)
> > > a patch - which should cause a discussion about resolving the veto.
> > >
> > > So, #1 - your suggestion sounds right with the need for three
> committers to
> > > approve before merge to develop.
> > >
> > > For #2, I think it should be a separate branch and require 3 signoffs
> for
> > > now.
> > >
> > > As the project matures, "obvious" commits can be CTR.
> > >
> > > My $.02. -- justin
> > > On May 7, 2015 5:44 AM, "Pid" <pi...@pidster.com> wrote:
> > >
> > >> Hi,
> > >>
> > >>
> > >> Like it says, can we discuss how the review process will work?
> > >> For these examples:
> > >>
> > >>
> > >> 1. I would like to work on upgrading the Spring dependencies in gfsh.
> > >>
> > >> Proposed approach: file a JIRA, cut a feature branch, push it & then
> what?
> > >>
> > >>
> > >> 2. I would like to add an entry to .gitignore (.idea/)
> > >>
> > >> Does this require a JIRA, a feature branch and a review?
> > >>
> > >>
> > >>
> > >> p
> > >>
> > >> --
> > >>
> > >> [key:62590808]
> > >>
> >
>
--
William Markito Oliveira
Enterprise Architect
*http://www.pivotal.io/ <http://www.pivotal.io/>*
For prompt responses to questions on GemFire/GemFireXD, please write
to *rtds-dev-ea
at pivotal.io <http://pivotal.io>*
Re: [DISCUSS] Review process + Git Flow
Posted by Konstantin Boudnik <co...@apache.org>.
RTC is great, but it tends to slow down the contribution process (for existing
committers) and also might (in extreme cases) lead to a low-quality reviews,
as it adds more work on developers. CTR on the other hand, will let the
changes to go in much faster but it requires a few things:
- mature committers who won't commit crap into the common code base
- well-oiled CI to ensure sufficient testing of new commits
- optionally, a longer time to gain the commit-bit
Spark example is a bit extreme and I don't think it flies well with the
board@ nor it is really compatible with open-community model.
There's nothing wrong in having maintainers (for once we have it in Bigtop),
but it is certainly wrong to promote these maintainers into the status of a
component kings.
Regards,
Cos
On Thu, May 07, 2015 at 07:13AM, Anthony Baker wrote:
> Agree that RTC is really important. In addition, we should consider that
> some changes require specific knowledge and context (I’m thinking of you
> AbstractRegionMap). Note that I’m not advocating for code ownership. Spark
> [1] uses this approach:
>
> "For certain modules, changes to the architecture and public API should also
> be reviewed by a maintainer for that module (which may or may not be the
> same as the main reviewer) before being merged. The PMC has designated the
> following maintainers…”
>
> Changes to public API’s or core internals would fall into this category. Thoughts?
>
>
> Anthony
>
> [1] https://cwiki.apache.org/confluence/display/SPARK/Committers
>
>
>
> > On May 7, 2015, at 3:38 AM, Justin Erenkrantz <ju...@erenkrantz.com> wrote:
> >
> > One question that we need to discuss is whether every merge is RTC
> > (Review-than-Commit) or CTR (Commit-than-Review).
> >
> > My take is that we should start with RTC and, if the review process gets in
> > the way of innovation, then we go to CTR. But, until everyone learns the
> > rules of the road, I think RTC is justified. Under RTC rules, all commits
> > should be reviewed (+1) by three committers before being merged. (If you
> > are a committer, then two others are needed.). Any committer can veto (-1)
> > a patch - which should cause a discussion about resolving the veto.
> >
> > So, #1 - your suggestion sounds right with the need for three committers to
> > approve before merge to develop.
> >
> > For #2, I think it should be a separate branch and require 3 signoffs for
> > now.
> >
> > As the project matures, "obvious" commits can be CTR.
> >
> > My $.02. -- justin
> > On May 7, 2015 5:44 AM, "Pid" <pi...@pidster.com> wrote:
> >
> >> Hi,
> >>
> >>
> >> Like it says, can we discuss how the review process will work?
> >> For these examples:
> >>
> >>
> >> 1. I would like to work on upgrading the Spring dependencies in gfsh.
> >>
> >> Proposed approach: file a JIRA, cut a feature branch, push it & then what?
> >>
> >>
> >> 2. I would like to add an entry to .gitignore (.idea/)
> >>
> >> Does this require a JIRA, a feature branch and a review?
> >>
> >>
> >>
> >> p
> >>
> >> --
> >>
> >> [key:62590808]
> >>
>
Re: [DISCUSS] Review process + Git Flow
Posted by Pid <pi...@pidster.com>.
On 07/05/2015 15:24, Dan Smith wrote:
> +1 For review then commit
>
> +1 For designating some maintainers for certain modules - as long as there
> are no components with only 1 maintainer.
>
> Also, I think runnning all the tests for any commit that affects source
> should be a requirement.
>From which I wonder, can we set up branch based CI? That would take any
uncertainty out and remove the burden of a reviewer to run the tests.
> Should we require any period of time for additional review comments, or can
> the commit be merged as soon as it has been approved by enough committers?
That's an interesting question. A cooling off period might be good.
Also worth considering what happens in the event a proposal is not
reviewed for a while.
> -Dan
>
> On Thu, May 7, 2015 at 7:13 AM, Anthony Baker <ab...@pivotal.io> wrote:
>
>> Agree that RTC is really important. In addition, we should consider that
>> some changes require specific knowledge and context (I’m thinking of you
>> AbstractRegionMap). Note that I’m not advocating for code ownership.
>> Spark [1] uses this approach:
>>
>> "For certain modules, changes to the architecture and public API should
>> also be reviewed by a maintainer for that module (which may or may not be
>> the same as the main reviewer) before being merged. The PMC has designated
>> the following maintainers…”
>>
>> Changes to public API’s or core internals would fall into this category.
>> Thoughts?
>>
>>
>> Anthony
>>
>> [1] https://cwiki.apache.org/confluence/display/SPARK/Committers
>>
>>
>>
>>> On May 7, 2015, at 3:38 AM, Justin Erenkrantz <ju...@erenkrantz.com>
>> wrote:
>>>
>>> One question that we need to discuss is whether every merge is RTC
>>> (Review-than-Commit) or CTR (Commit-than-Review).
>>>
>>> My take is that we should start with RTC and, if the review process gets
>> in
>>> the way of innovation, then we go to CTR. But, until everyone learns the
>>> rules of the road, I think RTC is justified. Under RTC rules, all
>> commits
>>> should be reviewed (+1) by three committers before being merged. (If you
>>> are a committer, then two others are needed.). Any committer can veto
>> (-1)
>>> a patch - which should cause a discussion about resolving the veto.
>>>
>>> So, #1 - your suggestion sounds right with the need for three committers
>> to
>>> approve before merge to develop.
>>>
>>> For #2, I think it should be a separate branch and require 3 signoffs for
>>> now.
>>>
>>> As the project matures, "obvious" commits can be CTR.
>>>
>>> My $.02. -- justin
>>> On May 7, 2015 5:44 AM, "Pid" <pi...@pidster.com> wrote:
>>>
>>>> Hi,
>>>>
>>>>
>>>> Like it says, can we discuss how the review process will work?
>>>> For these examples:
>>>>
>>>>
>>>> 1. I would like to work on upgrading the Spring dependencies in gfsh.
>>>>
>>>> Proposed approach: file a JIRA, cut a feature branch, push it & then
>> what?
>>>>
>>>>
>>>> 2. I would like to add an entry to .gitignore (.idea/)
>>>>
>>>> Does this require a JIRA, a feature branch and a review?
>>>>
>>>>
>>>>
>>>> p
>>>>
>>>> --
>>>>
>>>> [key:62590808]
>>>>
>>
>>
>
--
[key:62590808]
Re: [DISCUSS] Review process + Git Flow
Posted by Dan Smith <ds...@pivotal.io>.
+1 For review then commit
+1 For designating some maintainers for certain modules - as long as there
are no components with only 1 maintainer.
Also, I think runnning all the tests for any commit that affects source
should be a requirement.
Should we require any period of time for additional review comments, or can
the commit be merged as soon as it has been approved by enough committers?
-Dan
On Thu, May 7, 2015 at 7:13 AM, Anthony Baker <ab...@pivotal.io> wrote:
> Agree that RTC is really important. In addition, we should consider that
> some changes require specific knowledge and context (I’m thinking of you
> AbstractRegionMap). Note that I’m not advocating for code ownership.
> Spark [1] uses this approach:
>
> "For certain modules, changes to the architecture and public API should
> also be reviewed by a maintainer for that module (which may or may not be
> the same as the main reviewer) before being merged. The PMC has designated
> the following maintainers…”
>
> Changes to public API’s or core internals would fall into this category.
> Thoughts?
>
>
> Anthony
>
> [1] https://cwiki.apache.org/confluence/display/SPARK/Committers
>
>
>
> > On May 7, 2015, at 3:38 AM, Justin Erenkrantz <ju...@erenkrantz.com>
> wrote:
> >
> > One question that we need to discuss is whether every merge is RTC
> > (Review-than-Commit) or CTR (Commit-than-Review).
> >
> > My take is that we should start with RTC and, if the review process gets
> in
> > the way of innovation, then we go to CTR. But, until everyone learns the
> > rules of the road, I think RTC is justified. Under RTC rules, all
> commits
> > should be reviewed (+1) by three committers before being merged. (If you
> > are a committer, then two others are needed.). Any committer can veto
> (-1)
> > a patch - which should cause a discussion about resolving the veto.
> >
> > So, #1 - your suggestion sounds right with the need for three committers
> to
> > approve before merge to develop.
> >
> > For #2, I think it should be a separate branch and require 3 signoffs for
> > now.
> >
> > As the project matures, "obvious" commits can be CTR.
> >
> > My $.02. -- justin
> > On May 7, 2015 5:44 AM, "Pid" <pi...@pidster.com> wrote:
> >
> >> Hi,
> >>
> >>
> >> Like it says, can we discuss how the review process will work?
> >> For these examples:
> >>
> >>
> >> 1. I would like to work on upgrading the Spring dependencies in gfsh.
> >>
> >> Proposed approach: file a JIRA, cut a feature branch, push it & then
> what?
> >>
> >>
> >> 2. I would like to add an entry to .gitignore (.idea/)
> >>
> >> Does this require a JIRA, a feature branch and a review?
> >>
> >>
> >>
> >> p
> >>
> >> --
> >>
> >> [key:62590808]
> >>
>
>
Re: [DISCUSS] Review process + Git Flow
Posted by Anthony Baker <ab...@pivotal.io>.
Agree that RTC is really important. In addition, we should consider that some changes require specific knowledge and context (I’m thinking of you AbstractRegionMap). Note that I’m not advocating for code ownership. Spark [1] uses this approach:
"For certain modules, changes to the architecture and public API should also be reviewed by a maintainer for that module (which may or may not be the same as the main reviewer) before being merged. The PMC has designated the following maintainers…”
Changes to public API’s or core internals would fall into this category. Thoughts?
Anthony
[1] https://cwiki.apache.org/confluence/display/SPARK/Committers
> On May 7, 2015, at 3:38 AM, Justin Erenkrantz <ju...@erenkrantz.com> wrote:
>
> One question that we need to discuss is whether every merge is RTC
> (Review-than-Commit) or CTR (Commit-than-Review).
>
> My take is that we should start with RTC and, if the review process gets in
> the way of innovation, then we go to CTR. But, until everyone learns the
> rules of the road, I think RTC is justified. Under RTC rules, all commits
> should be reviewed (+1) by three committers before being merged. (If you
> are a committer, then two others are needed.). Any committer can veto (-1)
> a patch - which should cause a discussion about resolving the veto.
>
> So, #1 - your suggestion sounds right with the need for three committers to
> approve before merge to develop.
>
> For #2, I think it should be a separate branch and require 3 signoffs for
> now.
>
> As the project matures, "obvious" commits can be CTR.
>
> My $.02. -- justin
> On May 7, 2015 5:44 AM, "Pid" <pi...@pidster.com> wrote:
>
>> Hi,
>>
>>
>> Like it says, can we discuss how the review process will work?
>> For these examples:
>>
>>
>> 1. I would like to work on upgrading the Spring dependencies in gfsh.
>>
>> Proposed approach: file a JIRA, cut a feature branch, push it & then what?
>>
>>
>> 2. I would like to add an entry to .gitignore (.idea/)
>>
>> Does this require a JIRA, a feature branch and a review?
>>
>>
>>
>> p
>>
>> --
>>
>> [key:62590808]
>>
Re: [DISCUSS] Review process + Git Flow
Posted by Roman Shaposhnik <ro...@shaposhnik.org>.
On Thu, May 7, 2015 at 3:38 AM, Justin Erenkrantz <ju...@erenkrantz.com> wrote:
> One question that we need to discuss is whether every merge is RTC
> (Review-than-Commit) or CTR (Commit-than-Review).
>
> My take is that we should start with RTC and, if the review process gets in
> the way of innovation, then we go to CTR. But, until everyone learns the
> rules of the road, I think RTC is justified. Under RTC rules, all commits
> should be reviewed (+1) by three committers before being merged. (If you
> are a committer, then two others are needed.). Any committer can veto (-1)
> a patch - which should cause a discussion about resolving the veto.
>
> So, #1 - your suggestion sounds right with the need for three committers to
> approve before merge to develop.
>
> For #2, I think it should be a separate branch and require 3 signoffs for
> now.
>
> As the project matures, "obvious" commits can be CTR.
I am very much a +1 on the above suggestion.
Thanks,
Roman.
Re: [DISCUSS] Review process + Git Flow
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
One question that we need to discuss is whether every merge is RTC
(Review-than-Commit) or CTR (Commit-than-Review).
My take is that we should start with RTC and, if the review process gets in
the way of innovation, then we go to CTR. But, until everyone learns the
rules of the road, I think RTC is justified. Under RTC rules, all commits
should be reviewed (+1) by three committers before being merged. (If you
are a committer, then two others are needed.). Any committer can veto (-1)
a patch - which should cause a discussion about resolving the veto.
So, #1 - your suggestion sounds right with the need for three committers to
approve before merge to develop.
For #2, I think it should be a separate branch and require 3 signoffs for
now.
As the project matures, "obvious" commits can be CTR.
My $.02. -- justin
On May 7, 2015 5:44 AM, "Pid" <pi...@pidster.com> wrote:
> Hi,
>
>
> Like it says, can we discuss how the review process will work?
> For these examples:
>
>
> 1. I would like to work on upgrading the Spring dependencies in gfsh.
>
> Proposed approach: file a JIRA, cut a feature branch, push it & then what?
>
>
> 2. I would like to add an entry to .gitignore (.idea/)
>
> Does this require a JIRA, a feature branch and a review?
>
>
>
> p
>
> --
>
> [key:62590808]
>