Code reviews, UX, and tests

[Gian Merlino <gi...@apache.org>]

Hey Druids,

I am writing to you all to ask for your help 🙂

In particular, your help in ensuring that potential code contributions are
reviewed in a timely fashion. Right now we have 72 open PRs, which due to
stalebot are mostly opened pretty recently. That's a lot of people that
want to contribute stuff!

First, I wanted to point out that a lot of the reviews are done by a
relatively small number of people. If you are one of those people, thank
you. If you aren't, please consider becoming one.

Second, I've been thinking about how to make reviewing a little easier.
Maybe this will help more people become reviewers. I'd like to suggest
focusing on user experience and tests.

1) "User experience" means any new properties being introduced, any new
features being introduced, any behavior changes to existing features.

2) "Tests" are ideally automated tests that run as part of Travis CI. But
there are other kinds of tests too.

It's important to get user experience right, because if we release a
feature with poor UX, then it's hard to change later without breaking
compatibility. We'd like the master branch to be releasable at all times,
which means we should figure this stuff out before committing a patch.

It's also important to get tests right, because they ensure high quality
releases, and they also ensure that future changes can be made with low
risk.

It's less important to get other code-level things right, because we can
always change them later, and if there aren't UX or quality impacts then no
harm was done.

For contributors, focusing on UX and tests means writing out (in natural
language) how your patch changes user experience, and why you think this
change is a good idea. It also means having good testing of the new stuff
you're adding, and writing out (in natural language) why you think your
tests cover all the important cases. Speaking as a person that has reviewed
a lot of code: these natural language descriptions are *very helpful*,
especially when they add context to the patch. Don't make reviewers
reverse-engineer your code to guess what you were thinking.

For reviewers, it means that if we are confident in the UX and testing, we
don't need to spend a ton of time poring over every line of code. (I'd
still take the time to review key classes and interfaces, but this doesn't
take as much time as reviewing every line.) It also means that if we get a
PR that isn't set up to enable quick review, then we should write that to
the contributor and invite them to improve their PR, rather than ignoring
it or spending too much time on it. (Of course, we should only ask for this
if we can actually follow up with a review when the PR is improved later
on.)

I'd love to hear what people think.

Gian

Re: Code reviews, UX, and tests

[Keefe Roedersheimer <ke...@verizonmedia.com.INVALID>]

Hi Gian, 

First off, thank you (and everyone) for all your hard work on Druid. It's been great to work with over the past few months and thank you for giving some guidance on how to best collaborate with the community. 

I think it makes a lot of sense to focus on UX to avoid backward compatibility issues. We have similar discussions here about solving problems for our internal customers while avoiding backward compatibility issues in custom versions and instead contributing back to the community as much as possible. 

For some context, we have several reasonably large clusters across the company for different purposes, on the order of hundreds of historicals each. We get problem reports or feature requests from our users that we have bandwidth to do, but we don't want to end up with many patches that have to be ported every time Druid upgrades. 

The strategy we came up with so far is to create an issue that describes the problem or feature once we can accurately reproduce and/or describe the issue. After that, we've continued working the problem and then started a PR once we got to what seemed like a reasonable solution. I just sent an example to this list for reference. Obviously, it is no problem rewriting a PR or taking a totally different approach. Generally, we've tried to make as minimal change to existing code as possible and use extensions as much as possible. It would be awesome to hear any more feedback for how to best work with the community!

Keefe

On 2020/10/15 09:21:28, Gian Merlino <gi...@apache.org> wrote: 
> Hey Druids,
> 
> I am writing to you all to ask for your help 🙂
> 
> In particular, your help in ensuring that potential code contributions are
> reviewed in a timely fashion. Right now we have 72 open PRs, which due to
> stalebot are mostly opened pretty recently. That's a lot of people that
> want to contribute stuff!
> 
> First, I wanted to point out that a lot of the reviews are done by a
> relatively small number of people. If you are one of those people, thank
> you. If you aren't, please consider becoming one.
> 
> Second, I've been thinking about how to make reviewing a little easier.
> Maybe this will help more people become reviewers. I'd like to suggest
> focusing on user experience and tests.
> 
> 1) "User experience" means any new properties being introduced, any new
> features being introduced, any behavior changes to existing features.
> 
> 2) "Tests" are ideally automated tests that run as part of Travis CI. But
> there are other kinds of tests too.
> 
> It's important to get user experience right, because if we release a
> feature with poor UX, then it's hard to change later without breaking
> compatibility. We'd like the master branch to be releasable at all times,
> which means we should figure this stuff out before committing a patch.
> 
> It's also important to get tests right, because they ensure high quality
> releases, and they also ensure that future changes can be made with low
> risk.
> 
> It's less important to get other code-level things right, because we can
> always change them later, and if there aren't UX or quality impacts then no
> harm was done.
> 
> For contributors, focusing on UX and tests means writing out (in natural
> language) how your patch changes user experience, and why you think this
> change is a good idea. It also means having good testing of the new stuff
> you're adding, and writing out (in natural language) why you think your
> tests cover all the important cases. Speaking as a person that has reviewed
> a lot of code: these natural language descriptions are *very helpful*,
> especially when they add context to the patch. Don't make reviewers
> reverse-engineer your code to guess what you were thinking.
> 
> For reviewers, it means that if we are confident in the UX and testing, we
> don't need to spend a ton of time poring over every line of code. (I'd
> still take the time to review key classes and interfaces, but this doesn't
> take as much time as reviewing every line.) It also means that if we get a
> PR that isn't set up to enable quick review, then we should write that to
> the contributor and invite them to improve their PR, rather than ignoring
> it or spending too much time on it. (Of course, we should only ask for this
> if we can actually follow up with a review when the PR is improved later
> on.)
> 
> I'd love to hear what people think.
> 
> Gian
> 

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@druid.apache.org
For additional commands, e-mail: dev-help@druid.apache.org