Mailing-List: contact dev-help@asterixdb.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@asterixdb.apache.org
Content-Type: text/plain; charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 9.3 \(3124\))
Subject: Re: [DISCUSS] Improving reviews
From: abdullah alamoudi <bamousaa@gmail.com>
In-Reply-To: <5694B0DB-FEDC-4031-A341-DAA2DA2B277D@gmail.com>
Date: Tue, 6 Dec 2016 10:42:59 -0800
Content-Transfer-Encoding: quoted-printable
Message-Id: <8AD2E0F6-43EE-4006-BE60-E5136C4AE2B7@gmail.com>
References: <D14919E0-405E-458A-AE96-425C5490ECF6@apache.org> <CAHWyyRw7qCzhuse4nBuEq7Rj+uZ3UK21_N+ADhVu3qGb7+Ezfg@mail.gmail.com> <5694B0DB-FEDC-4031-A341-DAA2DA2B277D@gmail.com>
To: dev@asterixdb.apache.org,
 chillery@hillery.land
archived-at: Tue, 06 Dec 2016 18:43:14 -0000

I would like to stress one point though: Having no comments after a =
timeout (72hr) means that the reviewer couldn't find time to do the =
review. In which case, the change owner needs to be extra careful as the =
whole blame will be on them if their change breaks something. If you're =
not totally sure about your change and are not testing every little =
possible case, you can still insist on a review.

One issue that comes to mind is: what if someone's changes continuously =
break things?
Of course we can't revoke commit privileges (Can we?) but what we can do =
is:
1. pay more attention to changes submitted by people who's changes break =
more often.
2. increase the timeout period for them "temporarily" 72->96->120..... =
If you don't like this, don't make bad changes.
3. MOST IMPORTANTLY,  all of us should strive to have better test =
coverage to automatically detect breaks caused by wild changes.

Cheers,
~Abdullah.

> On Dec 6, 2016, at 10:33 AM, abdullah alamoudi <bamousaa@gmail.com> =
wrote:
>=20
> Ceej,
> You spoke my mind and I agree with every word. I believe the way to go =
is smoother code review process (maybe through time limit) and more =
focus on automated testing. One thing we could do is:
> If the 72 hours period pass with no comments, we look at the code =
coverage and how much it improved and based on that, either allow the =
change in or deny it. This will push change submitters to add tests to =
increase coverage even before the 72h limit passes. This doesn't remove =
the responsibility of doing the review. The review is still to be done. =
However as Ceej said: One of the goals of doing the reviews is to catch =
large scale design errors. Those I think still need humans to look at =
but they can be caught fairly quickly with minimal effort.
>=20
> As for spreading the knowledge, will leave that to a different =
discussion. I will end this with some tweets about code simplicity and =
changes taken from Max Kanat-Alexander, author of Code Simplicity:
>=20
> 1. You don't have to be perfect. If you make a bad change, just fix =
it. (mistakes will happen with or without reviews)
> 2. If somebody is improving code quality, don't shoot them down =
because their improvement isn't perfect. (to reviewers)
> 3. The point is to have a maintainable system, not to show how clever =
you are. (to submitters)
> 4. Code quality isn't something you fix once and it stays good =
forever. It's something you learn to do and continue doing.
> 5. Engineers don't beg, "Please let me build a bridge that will stay =
up." You shouldn't need permission to write good software.
> 6. Anybody who tells you that you can fix years of bad code in months =
or days is a liar.
> 7. Even huge codebases can be refactored incrementally.
> 8. Sometimes a refactoring will break something. Often, this proves =
that the code was too fragile and so needed the refactoring!
> 9. If your code "works," but it's an unstable pile of complexity, do =
you feel good about it?
> 10. Refactoring is often easier and more rewarding than you expect.
> 11. Don't try to write "perfect" code, just write *better* code until =
you have *good* code.
> 12. Don't worry about how to do the perfect refactoring. Just keep =
improving the code in small steps.
>=20
> I am glad we're talking about this. Cheers,
> ~Abdullah.
>=20
>> On Dec 5, 2016, at 11:13 PM, Chris Hillery <chillery@hillery.land> =
wrote:
>>=20
>> It's always been my opinion that code reviews are a very =
nice-to-have, but
>> not more than that. The real value in proposing changes for review =
comes
>> from the automated testing that can be performed at that stage. I =
think
>> we'd be better served overall by shoring up and expanding our =
automated
>> testing rather than spending time discussing and implementing =
non-technical
>> process.
>>=20
>> The main benefits of code reviews are catching large-scale design =
errors
>> and spreading code knowledge. You can't really have the former until =
you
>> already have the latter - if only one person really understands an =
area,
>> nobody else will be able to catch design errors in that area. That's
>> clearly a risky place to be, but IMHO at least it's not a problem =
that can
>> be solved through rules. It requires a cultural shift from the team =
to make
>> spreading code knowledge an actual priority, rather than someone =
everyone
>> wants but nobody has time or interest to achieve.
>>=20
>> If we as a team don't have the drive to do that, then we should =
accept that
>> about ourselves and move on. You'll always do best spending time on
>> enhancing the strengths of a team, not fighting against the things =
they
>> don't excel at. I'm also not trying to make any kind of value =
judgment here
>> - software development is always about tradeoffs and compromise, risk
>> versus goals. Any time taken to shift focus towards spreading code
>> knowledge will by necessity pull from other parts of the development
>> process, and the upshot may well not be an overall improvement in
>> functionality or quality.
>>=20
>> Ceej
>> aka Chris Hillery
>>=20
>> On Mon, Dec 5, 2016 at 10:49 PM, Till Westmann <tillw@apache.org> =
wrote:
>>=20
>>> Hi,
>>>=20
>>> today a few of us had a discussion about how we could make the =
reviewing
>>> process moving along a little smoother. The goal is to increase the
>>> likeliness
>>> that the reviews and review comments get addressed reasonably =
quickly. To
>>> do
>>> that, the proposal is to
>>> a) try to keep ourselves to some time limit up to which a reviewer =
or
>>> author
>>>  responds to a review or a comment and to
>>> a) regularly report via e-mail about open reviews and how long they =
have
>>> been
>>>  open (Ian already has filed an issue to automate this [1]).
>>> Of course one is not always able to spend the time to do a thorough =
review
>>> [2]
>>> / respond fully to comments, but in this case we should aim to let =
the
>>> other
>>> participants know within the time limit that the task is not =
feasible so
>>> that
>>> they adapt their plan accordingly.
>>> The first proposal for the time limit would be 72h (which is taken =
from the
>>> minimal time that a [VOTE] stays open to allow people in all =
different
>>> locations and timezones to vote).
>>> Another goal would be to abandon reviews, if nobody seems to be =
working on
>>> them
>>> for a while (and we=E2=80=99d need to find out what "a while" could =
be).
>>>=20
>>> Thoughts on this?
>>> A good idea or too much process?
>>> Is the time limit reasonable?
>>> Please let us know what you think (ideally more than a +1 or a -1 =
...)
>>>=20
>>> Cheers,
>>> Till
>>>=20
>>> [1] https://issues.apache.org/jira/browse/ASTERIXDB-1745
>>> [2] =
https://cwiki.apache.org/confluence/display/ASTERIXDB/Code+Reviews
>>>=20
>=20