incubator-general mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Harbs <harbs.li...@gmail.com>
Subject Re: RTC vs CTR (was: Concerning Sentry...)
Date Wed, 25 Nov 2015 22:12:28 GMT

On Nov 25, 2015, at 11:32 PM, Ralph Goers <ralph.goers@dslextreme.com> wrote:

> 1. What makes you think all bugs are caught during code reviews (they aren’t)?

I don’t, and I did not infer that.

> 2. What makes you think that code reviews after the commit are any less thorough than
reviews required before the commit?

Nothing. I did not say that. If the code review is done, it makes no difference when. I only
said that CTR insures that the CODE REVIEW IS ACTUALLY DONE.

> If you don’t trust your community to do code reviews after you commit then there is
a problem in your community. Forcing a code review to occur first won’t fix that.
> 
I don’t see it as not trusting the community. I think Flex is doing just fine right now.
But Flex is a big code base and there’s areas where not a lot of people work on it. Especially,
on a big code-base, there’s people working on different things. It’s totally reasonable
for something to go under everyone’s radar — especially in an area of the code where there
are not a lot of people working on it. Mandatory code reviews seems to me a method of making
sure that it doesn’t get missed. If the code doesn’t get reviewed after x number of days,
the person who's committing can send an email to the list asking for someone to look it over.
What’s wrong with that?

> In a CTR world you can choose to do every piece of work on a branch and ask for a code
review before you commit. That is your choice.  But if you know that the code you are committing
is good because you have a thorough knowledge of your product you shouldn’t be forced to
have it reviewed before you can commit it.  I actually love the line in Kudu where it says
that automation insures quality. I am a big fan of that. In my experience having lots of tests
is the best way to insure stuff doesn’t get broken.
> 
> So how did you catch the bugs in your code in Flex?  Would you have preferred that they
stay on a branch for months so they could be reviewed before committing?  Despite how great
people say git is I have still had lots of problems resolving merge conflicts when the code
isn’t merged back quickly.  If I understand what you are saying you would prefer that Flex
use RTC because you don’t trust your fellow committers to review your code.  That is a community
problem that needs to be fixed. Forcing them to review the code first isn’t the proper way
to fix it.

The bugs were found after the last release of Flex and reported by users.

No. I’m not saying I want RTC. I don’t. I’m quite happy with CTR in my community. Small
bugs in Flex even if not caught will not likely cause users millions of dollars. I’m okay
if there might be more bugs in Flex and not requiring code review, because code review DOES
make things more difficult. All I’m saying is that I understand the rationale as quality
assurance for communities who consider the damage for regressive bugs to be very high. It
seems like a certain amount of RTC can be a reasonable price to pay.


> Ralph
> 
>> On Nov 25, 2015, at 2:00 PM, Harbs <harbs.lists@gmail.com> wrote:
>> 
>> If a review is required for non-code changes to the main branch, then I agree.
>> 
>> I’m sure you agree that reviews on code make for less bugs. We all make mistakes
and can overlook things. It seems kind of extreme to assume that this kind of required review
is all about control. Since anyone who can commit can review, it’s kind of hard for me to
swallow that.
>> 
>> I assume your logic is that the reviews can come after the commit. Sure. But what
if it doesn’t?
>> 
>> Case in point: I made some pretty major changes to TLF in Flex which constituted
a number of months of work. I’m willing to bet that not every commit I did was checked by
others. I did a decent job, but there were a few regressive bugs in my code, and I accidentally
reverted some code in my commit as well. In a workflow where my code would have to get one
or more +1s before I committed it to the main branch, it’s likely that the reverted commit
(at the least) would have been caught.
>> 
>> I would actually welcome knowing someone looked over my code for a sanity check.
>> 
>> Harbs
>> 
>> On Nov 25, 2015, at 10:49 PM, Greg Stein <gstein@gmail.com> wrote:
>> 
>>> That is pretty normal operation in both styles of workflow. My concern is
>>> with trunk/master. Is a committer trusted enough to make changes directly?
>>> 
>>> If all meaningful changes (ie. changing APIs and algorithms, not just
>>> fixing typos w/o review) are not trusted, and require review/permission,
>>> then I'm against that.
>>> 
>>> It is good practice to put potentially disruptive code onto a branch while
>>> it is developed, then merge it when complete. Trusting a committer to ask
>>> for review before the merge is great. Requiring it, less so.
>>> 
>>> But RTC on trunk/master is harmful.
>>> 
>>> Cheers,
>>> -g
>>> 
>>> On Wed, Nov 25, 2015 at 2:44 PM, Harbs <harbs.lists@gmail.com> wrote:
>>> 
>>>> What about commit to feature/bug brach, review and then commit to main
>>>> branch?
>>>> 
>>>> Is that CTR or RTC in your book?
>>>> 
>>>> On Nov 25, 2015, at 10:42 PM, Greg Stein <gstein@gmail.com> wrote:
>>>> 
>>>>> I object to Lucene's path, too. A committer's judgement is not trusted
>>>>> enough to make a change without upload/review. They need permission first
>>>>> (again: to use your term; it works great).
>>>>> 
>>>>> On Wed, Nov 25, 2015 at 2:39 PM, Upayavira <uv@odoko.co.uk> wrote:
>>>>> 
>>>>>> Some setups that people call RTC are actually CTR in your nomenclature,
>>>>>> so we could be talking cross-purposes. That's all I'm trying to avoid.
>>>>>> E.g. Lucene - everything happens in JIRA first (upload patch, wait
for
>>>>>> review), but once that has happened, you are free to commit away.
So
>>>>>> strictly, it is RTC, but not seemingly in the sense you are objecting
>>>>>> to.
>>>>>> 
>>>>>> Upayavira
>>>>>> 
>>>>>> On Wed, Nov 25, 2015, at 08:35 PM, Greg Stein wrote:
>>>>>>> I think this is a distraction. You said it best the other day:
RTC
>>>>>>> implies
>>>>>>> the need for "permission" before making a change to the codebase.
>>>>>>> Committers are not trusted to make a judgement on whether a change
>>>> should
>>>>>>> be made.
>>>>>>> 
>>>>>>> CTR trusts committers to use their judgement. RTC distrusts committers,
>>>>>>> and
>>>>>>> makes them seek permission [though one of several mechanisms].
>>>>>>> 
>>>>>>> -g
>>>>>>> 
>>>>>>> On Wed, Nov 25, 2015 at 10:47 AM, Upayavira <uv@odoko.co.uk>
wrote:
>>>>>>> 
>>>>>>>> Not replying to this mail specifically, but to the thread
in
>>>> general...
>>>>>>>> 
>>>>>>>> People keep using the terms RTC and CTR as if we all mean
the same
>>>>>>>> thing. Please don't. If you must use these terms, please
define what
>>>>>> you
>>>>>>>> mean by them.
>>>>>>>> 
>>>>>>>> CTR is a less ambiguous term - I'd suggest we all assume
that "commit"
>>>>>>>> means a push to a version control system.
>>>>>>>> 
>>>>>>>> However, RTC seems to mean many things - from "push to JIRA
for review
>>>>>>>> first, wait a bit, then commit to VCS" through "push to JIRA,
and once
>>>>>>>> you have sufficient +1 votes, you can commit" to "push to
JIRA for a
>>>>>>>> review, then another committer must commit it".
>>>>>>>> 
>>>>>>>> If we're gonna debate RTC, can we please describe which of
these we
>>>> are
>>>>>>>> talking about (or some other mechanism that I haven't described)?
>>>>>>>> Otherwise, we will end up endlessly debating over the top
of each
>>>>>> other.
>>>>>>>> 
>>>>>>>> Upayavira
>>>>>>>> 
>>>>>>>> On Wed, Nov 25, 2015, at 09:28 AM, Harbs wrote:
>>>>>>>>> AIUI, there’s two ways to go about RTC which is easier
in Git:
>>>>>>>>> 1) Working in feature/bug fix branches. Assuming RTC
only applies to
>>>>>> the
>>>>>>>>> main branch, changes are done in separate branches where
commits do
>>>>>> not
>>>>>>>>> require review. The feature/bug fix branch is then only
merged back
>>>>>> in
>>>>>>>>> after it had a review. The reason this is easier is because
>>>>>> branching and
>>>>>>>>> merging is almost zero effort in Git. Many Git workflows
don’t work
>>>>>> on
>>>>>>>>> the main branch anyway, so this is a particularly good
fit for those
>>>>>>>>> workflows.
>>>>>>>>> 2) Pull requests. Using pull requests, all changes can
be pulled in
>>>>>> with
>>>>>>>>> a single command.
>>>>>>>>> 
>>>>>>>>> I’ve personally never participated in RTC (unless you
count Github
>>>>>>>>> projects and before I was a committer in Flex), so it
could be I’m
>>>>>>>>> missing something.
>>>>>>>>> 
>>>>>>>>> Of course there’s nothing to ENFORCE that the commit
is not done
>>>>>> before a
>>>>>>>>> review, but why would you want to do that? That’s where
trust comes
>>>>>> to
>>>>>>>>> play… ;-)
>>>>>>>>> 
>>>>>>>>> Harbs
>>>>>>>>> 
>>>>>>>>> On Nov 25, 2015, at 4:08 AM, Konstantin Boudnik <cos@apache.org>
>>>>>> wrote:
>>>>>>>>> 
>>>>>>>>>> I don't think Git is particularly empowering RTC
- there's nothing
>>>>>> in
>>>>>>>> it that
>>>>>>>>>> requires someone to look over one's shoulder.
>>>>>>>>> 
>>>>>>>> 
>>>>>>>> ---------------------------------------------------------------------
>>>>>>>> To unsubscribe, e-mail: general-unsubscribe@incubator.apache.org
>>>>>>>> For additional commands, e-mail: general-help@incubator.apache.org
>>>>>>>> 
>>>>>>>> 
>>>>>> 
>>>>>> ---------------------------------------------------------------------
>>>>>> To unsubscribe, e-mail: general-unsubscribe@incubator.apache.org
>>>>>> For additional commands, e-mail: general-help@incubator.apache.org
>>>>>> 
>>>>>> 
>>>> 
>>>> 
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: general-unsubscribe@incubator.apache.org
>>>> For additional commands, e-mail: general-help@incubator.apache.org
>>>> 
>>>> 
>> 
> 
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: general-unsubscribe@incubator.apache.org
> For additional commands, e-mail: general-help@incubator.apache.org
> 


---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@incubator.apache.org
For additional commands, e-mail: general-help@incubator.apache.org


Mime
View raw message