osdir.com

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: 'Design Review' tag


Roman, thanks for the detailed explanation. I totally agree with it.

I would add something more here for especially emphasizing the part of
'design review'.
As Roman said, the PRs tagged with 'Design Review' usually include the
changes which are hard to undo, so, it's very important to make the best
decision we can.
And the best design decision can be found by only considering reasonable
evidences.

So, I would suggest to enforce writing a proposal for all PRs which should
be labelled 'Design Review'. We're currently doing it only if someone wants
to write, but it gives a lot of benefits as below:

- To write a detailed proposal, authors can have a chance to think about
their idea thoroughly including very details like APIs, configuration
names, or even some code level changes.
- We can have a chance that authors provide enough evidences for every
design decision, so that reviewers can check and verify them. I believe
that we can make the best decision in this process.
- Since it's only a proposal not containing any code changes yet, it's much
easier to change design decisions than changing codes.

Many other open source projects are also doing this. For example, Kafka has
the 'Kafka Improvement Proposals' system (
https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals).
For Rust, it needs to write an RFC for substantial changes (
http://rust-lang.github.io/rfcs/).

I think one of the reasons why people hesitate to write proposals in Druid
is the absence of the appropriate format. I think the format should include
at least the followings:

- The problem description and motivation
- Proposed change
  - Overview of the change
  - Details including public API changes, configuration changes, algorithm,
and so on
- Expected benefits and drawbacks
- Rationale and alternatives

Welcome any idea.

Best,
Jihoon

On Fri, Oct 26, 2018 at 11:03 PM Roman Leventov <leventov@xxxxxxxxxx> wrote:

> Recently Charles (as far as I remember) asked what is the criteria for
> giving a PR 'Design Review' tag.
>
> I suggest that *a PR should be labelled "Design Review" if it will be hard
> to undo it (after it appears in some release), it will have lasting
> consequences on the codebase.*
>
> Addition of a new config property, public API, or changing existing public
> API is always hard to undo because it immediately becomes a subject for
> backwards compatibility. But some large code changes, even without
> user-facing API or config changes, could have similar effect on the
> codebase.
>
> Please keep in mind that it's not just for requiring two reviews instead of
> one, but also for really reviewing the design. E. g. bad config
> key/property names could stay forever because it's really hard to rename
> them, even harder than change Java APIs.
>