druid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Keefe Roedersheimer <keefe.roedershei...@verizonmedia.com.INVALID>
Subject Re: Code reviews, UX, and tests
Date Wed, 21 Oct 2020 19:18:23 GMT
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!


On 2020/10/15 09:21:28, Gian Merlino <gian@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

View raw message