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 <leven...@apache.org> 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. >
