Add the pull-request template

[leventov]

Adds the pull request template. Includes "Description" and "Design" sections.

In the bottom, the template contains a nudge for PR authors to review their patches themselves before publishing.

[jon-wei]

Suggest "Choice of algorithms" here instead

[leventov]

Applied

[jon-wei]

Suggest rewording to "Please describe any design decisions made, including: "

[leventov]

Applied

[jon-wei]

Suggest rewording to:

"Behavioral aspects (e.g., What configuration values are acceptable? How are corner cases and error conditions handled, such as when insufficient resources are available?)"

[leventov]

Applied

[jon-wei]

The template will need to be added to the Rat exclusions

[leventov]

@jon-wei added.

[leventov]

@asdf2014 @gianm could you please review this template?

[jihoonson]

I don't understand why this section exists. As far as I understand, every PR requiring design review should have a proposal according to the discussion on dev. Am I misunderstood?

[leventov]

Every code change has a design. Even choosing a name for a single variable is already an act of design. This section shouldn't discuss variable naming (although sometimes it can), but it can involve the following sentences:

• "This new query returns errors in cases of X, but returns empty results in case of Y, because ..."
• "I've used the factory (see MyNewCoolFactory) pattern instead of the builder pattern, because ..."

Real examples: in this change, I could (and probably should) have included the text from this comment into the Design section of the PR, because it's an act of naming design.

@egor-ryashin's #6349 is a good example, because the whole PR's description is actually description of the design of the change. Not only code design, but also the design of the solution. Where the feature is "we want to be able to decommission nodes safely", the design of the solution is "we provide a list of nodes to decommission to Coordinator via its dynamic config". This is very important and should always be described in the PR description.

What's also good about that description is that it presents the alternative design and compares it with the chosen design.

P. S. after writing this comment it seems to me that the sentence

Describe your patch: what did you change in code? How did you fix the problem?

From the "Description" section above should actually belong to the "Design" section.

[gianm]

There's a lot of overlap between this "Design" and the definition of a proposal: https://github.com/apache/incubator-druid/blob/master/.github/ISSUE_TEMPLATE/proposal.md. It's weird to have that template and this one talking about the same things from different angles. One way to address that is to take anything useful from this template that isn't already in the Proposal template, then merge that stuff into the Proposal template and simply reference that one here. And then open this template by saying that:

1. If you raised a proposal, just link to it.
2. If you didn't raise a proposal, think about doing that first, especially if the PR has any sort of 'interestingness' to its design that is worth discussing. If not, then include the same kind of information here in the PR.

I feel that encouraging people to raise proposals is useful because it promotes separation of design-level and implementation-level discussion. One point that was raised in the dev list when we talked about this is that commingling them can cause one to drown out the other.

[jihoonson]

@gianm's suggestion sounds good to me. Different aspects of design in different templates looks ambiguous.

[leventov]

I feel there is a fundamental problem that is behind this "separate Proposal issue" topic: we cannot really foresee what we will implement until we actually implement it. The final result may be drastically different from what we planned. The performance data may lead us to rethink the feature or its scope. The discussion in the Proposal may become irrelevant.

It's impossible to separate "design" and "implementation" (and therefore their discussions), neither in code, nor in time, nor in ideas.

This is why I think the PR template should contain the design section as well.

Maybe instead of

If you already did this in the associated issue
(e. g. a "Proposal" issue), leave the following sentence:

 Design of this change is discussed [here](<link to Github 
issue or comment where you discuss the design>).

at the end of the section we can add:

If there was a discussion of the feature implemented in this PR
elsewhere (e. g. a "Proposal" issue, any other issue, or a thread
in the development mailing list), link to that discussion and
explain what did change in your final design compared to your
original proposal or the consensus version in the end of the
discussion. If something hasn't changed since the original discussion,
you can omit a detailed discussion of those aspects of the design here,
perhaps apart from brief mentioning for the sake of readability of
this PR description.

It doesn't seem any practical to me to suggest creating separate issues for design discussions after implementing the whole change. Was it discussed?

[gianm]

I think there is value in separating them for PRs where a meaningful design discussion, separate from the code-level discussion, is anticipated. (Conversely: if no meaningful discussion is anticipated outside of the code level, then I don't see a point in separating the proposal from the PR.)

This is an example of one where I am finding it difficult to follow the design- and code-level discussions that are happening concurrently in the same PR: #7306. I think it would have benefited from a design proposal separate from the code-level changes.

[gianm]

Another way of putting this is that code-level discussions tend to get into the weeds pretty quickly, and a lot of code-level back and forth can too easily obscure a larger discussion about overall approach and design.

[leventov]

Does this mean that you disagree with the current wording of the template?

[gianm]

I think the "Design" section is good in the current draft. I do think we could use some cleaning up of our community guidelines around when a proposal is and isn't needed. But that doesn't need to be done as part of this PR to create a template.

I asked #7206 (comment) mainly because of the two checklists (general & concurrency), which I think need to be clarified as to whether it's meant to be expressing requirements or suggestions. My preference would be 'suggestions' because I think it's best if the list of requirements is kept short, and the concurrency checklist is quite long.

[jihoonson]

I don't think this is necessary. Providing a list of documents should be enough.

[leventov]

This is definitely provocative and pushes contributors out of their (non-self-reviewing) comfort zone, but this is the point. If there is just text "You can review your PR using this checklists: ...", I'm afraid, barely anybody will actually do this.

What do you think about the following wording?

### Self-review

(Did you self-review your PR?

Additionally, if your PR has any relation to concurrency (...), please use [this checklist](...).)

[leventov]

Note that I'm not acting as an exemplar here: in fact, I've only self-reviewed several (at best) my PRs into Druid, out of 150+ PRs, despite putting together the checklist. I've only once really carefully reviewed an already existing class with the checklist (that resulted in finding a bunch of bugs).

But I hope that adding this nudge will help people including myself to adopt this practice consistently because I think it's very powerful.

[leventov]

Or rather, I would do what @clintropolis suggests here: #7206 (comment) because it's elegant and promotes other good practices, apart from self-review.

[gianm]

I would prefer if this review checklist was removed from this PR and then potentially re-added separately from the act of creating a pull request template. This is because if we do end up having an official guide / checklist it should be in the main Druid repo, rather than a separate repo. It will make it easier to discuss its specific contents and evolve it over time in a way that reflects community consensus.

[jon-wei]

The project depends (with various meanings attached to "depends") on many non-Apache things, outside of any control by Druid committers, such as TeamCity, Travis, Github (all proprietary products), and dozens of non-Apache library dependencies.

What would be the alternative to using TeamCity, Travis, Github, or various dependencies? It would be an immense amount of effort to reimplement those tools or libraries.

Here, we would simply need to reformat some text and make a new doc page. I don't see sufficient justification for the indirection and complexity of having part of the project's official policies reside in an external repo.

All these dependencies seem to be "stronger" than the checklist that is not enforced in any way because ultimately committers review code themselves.

Can we rephrase the template to make the checklist recommended reading, and not make it sound like an official requirement then? If we do that, then I think it's fine to not bring the checklist into the project.

For example, a codebase already has several links to blog posts at https://shipilev.net as a "source of truth" explaining some things

I searched the code for that and see references in BitmapOffset and EventReceiverFirehoseFactory, and those simply provide context and technical explanation, they're not rules governing the committers or contributors.

[leventov]

What would be the alternative to using TeamCity, Travis, Github, or various dependencies? It would be an immense amount of effort to reimplement those tools or libraries.

We are not doing not because this would be an immense amount of effort, but because this is simply not needed.

Here, we would simply need to reformat some text and make a new doc page. I don't see sufficient justification for the indirection and complexity of having part of the project's official policies reside in an external repo.

I already explained why practically the checklist should be in a separate repo on its own. In the current form, it can be wherever, but the current form is not digestible for reviews. Making it more easily navigable likely requires some Jekyll magic that is not compatible with Druid repo.

[jon-wei]

What are your thoughts on positioning the checklist as a recommended resource instead of as an official-sounding part of the PR submission process/Druid codestyle?

I'm fine with having it in an external repo in that case.

[jon-wei]

The project depends (with various meanings attached to "depends") on many non-Apache things, outside of any control by Druid committers, such as TeamCity, Travis, Github (all proprietary products), and dozens of non-Apache library dependencies. All these dependencies seem to be "stronger" than the checklist that is not enforced in any way because ultimately committers review code themselves.

While the project doesn't entirely control external tools/infra, the project does control in-repo what tools like TeamCity and Travis do. The inspection profiles, style and formatting definitions, test suites, test setup scripts, etc. are in-repo.

I'm not arguing against having external dependencies in general, just specifically in the area of formal rules that govern the community, out of a belief that we should try to keep the project's governance as "clean" and self-contained as possible.

As written, the PR makes the checklist sound very official, and I don't think that concerns about confusion arising from having a fork of the checklist or formatting difficulties are sufficient justification for having an external dependency in this area.

[leventov]

I'll reword to make it recommended, not obligatory checklist.

[jihoonson]

If you think this is really important, I'm fine with adding it. But, this looks biased only toward concurrency issues which is a small part of general programming practices. I feel that it can mislead people to think that we care about only concurrency issues. Why don't we provide other documents as well for more general programming practices?

[leventov]

I'm currently relatively actively working on a more general (and more important) checklist about design and documentation, but it may still take a long time until I finish this work. Linking from the PR template to general programming books (even very good ones, like "Effective Java") is not actionable and practical, IMO.

I think it's better to include a link to the concurrency checklist for a while than not include any link at all.

I'll move a link to the concurrency checklist in a separate sentence.

[gianm]

I think it'd be better to skip the link completely. The checklist you have posted is long and complex and I am not sure how many Druid committers have read it fully and agree with it. It deserves its own separate discussion.

[leventov]

This checklist is "discussed" since November, but nobody participated the discussion except me and @egor-ryashin.

[gianm]

I'm suggesting raising a PR containing your checklist to the Druid repo as a way to encourage more discussion.

[leventov]

Gian, this is not a good basis for blocking this change because "was not discussed enough", although people had (and still have) all the opportunity to discuss it for 4+ months. I explained why I don't want to copy the checklist into the repo in this comment: #7206 (comment)

For example, in Apache, when discussing new committers, a discussion where nobody said anything is considered OK and then a vote thread is started anyway.

If you disagree with something in particular in this checklist, you can block this PR template PR for this reason. Then we can discuss the disagreements that should lead to some (or none) overriddings or amendments in the smaller document that I can create.

[gianm]

To my eyes, the language in the PR right now makes this checklist look like it's being represented as an official Druid style guide. If this is your intent with this PR, I don't think "external document with Druid-specific overrides" is a good approach, for a few reasons:

• It's tough for readers to manually reconcile. We have to ask people to read an external document and mentally apply diffs.
• It's tough for the Druid community to keep updated. We have to maintain a list of diffs rather than an actual document. If the upstream document changes, we have to update our diffs or "pin" to a specific version.
• No other projects seem to use the proposed checklist as part of their official style guide, so there isn't anything to gain from keeping it separate from Druid. Why not make an official Druid checklist and then, if other projects like it and want to use it, they can reference ours?

If your intent was not to represent this as an official Druid checklist, then I don't have as much issue with it, but in that case I think the language should be changed so it doesn't look so official. Maybe refer to this list as one of many possible resources on Java code style, and point out that Druid doesn't have an official coding guide, so contributors are encouraged to try to match the style of other code in the repo. (Basically how it works today.)

[leventov]

The idea behind your comment there will be some diffs. Actually, I don't think there will be anything in the Druid's document that is opposite to what the general checklist says. Because if the necessity for such points araises, then the general checklist is not general enough and the controversial points need to be softened or removed.

So, in fact, the Druid's document will contain only amendments (such as "Is it possible to use LifecycleLock instead of intrinsic lock for a lifecycled object?") or refinements, but not overridings. The users won't need to mentally apply diffs.

[gianm]

I assume there will be diffs at some point, since style guides aren't set in stone.

[leventov]

This is not a style (like, opinionated) checklist. Generality is really its goal. For example, pretty much all concurrency experts that I consulted with said that they have rules in their projects to prohibit daemon threads, but because Druid has the opposite policy, I've worded the corresponding item to not give preference to any approach.

[clintropolis]

Overall lgtm.

Would it be worth linking to CONTRIBUTING.md or incorporating some of it's contents as things to make sure the author has considered when opening a PR, like focusing to as few changes as possible to make review easier, etc? Ideally the authors have read that by this point, and are doing those things and it isn't necessary, but maybe would be prudent.

It might also be worth including a section that calls more attention to that if this PR is a large change they should be raising a proposal first.

[clintropolis]

Minor nit: I think the "only committers have the right to add tags" is implied by the "if you are a committer" part and can be left out.

Maybe also we should add a blurb to otherwise expect a committer to likely/eventually tag your PR for you as we work our way through to review things?

[leventov]

A blurb for the PR author (who is not a committer, in this case), or for the reviewing committers?

There is a "PR Management" section in the checklist draft. Perhaps we should extract it as a separate document "PR action item checklist for a committer" and publish to the wiki or the repository more promptly.

[gianm]

IMO better to say ability instead of right. It sounds a bit more respectful to my ears.

[jihoonson]

I think it's a definitely good idea to have a checklist, but I'm not sure if anyone tries to read them all if the checklist is 10+ pages long. Maybe the current checklist draft is too detailed. It would be better to be compact as much as possible.

[clintropolis]

I wonder if instead of instructing people to leave a sentence, we should incorporate a markdown checklist we recommend people fill out, with high level things things to do, including self review, maybe something like:

This PR has:

• been self-reviewed (recommended review checklist).
• added documentation for new or modified features or behaviors
• added Javadocs to public and non-trivial members
• added code comments for hard to understand areas
• added unit tests or modified existing tests to cover new code paths
• added integration tests
• been tested in a test environment
• been tested in a production environment

I don't know that all these things are necessary, or if anything is missing, and I think we would need to make clear that not all of these things are required. I was thinking mostly that it's a way for an author to convey to reviewers what has gone into the PR so far. We could suggest that PR authors are free to remove checks which are not applicable as well. Or maybe this is too much? I'm not sure, just throwing out ideas.

[leventov]

I really like this idea.

[gianm]

I like the spirit of a list like this but I think we need to be careful with the wording to avoid being confusing to newish contributors. It seems to suggest that the list of things is required for every PR. In fact, it's more like a list of things to consider doing if appropriate, rather than a list of requirements. Not every change needs to be tested in a production environment. Not every change needs integration tests, and in fact it's probably counter-productive to try (for lots of changes, the slowdown from adding integration tests isn't worth it).

[clintropolis]

I definitely agree that we would need to make clear that some items are optional and probably not recommended for most PRs or may not always be applicable, but as a reviewer this idea has grown on me quite a bit and I think I am totally for it in some form since I think it might help review go more smoothly. I would think the first 5 things are highly recommended, and 2-4 are probably required, integration tests if the behavior being or modified is very complex and has too many parts to capture in unit tests, and testing in clusters totally optional as a way for the author to communicate their own confidence in that the PR is in a good place and works as intended, should the reviewers approve the implementation.

[jihoonson]

Hmm, ok. This looks better to me. Maybe it would be better to add a non-applicable section for items which are not applicable and so ppl can move those items to that section?

[clintropolis]

I wonder if this should maybe instead be a wiki page in this repo, or some other official ASF resource?

[leventov]

I don't like this personal association myself, because I think it diminishes chances that people will use the checklist. I've moved it to https://github.com/code-review-checklists/java-concurrency.

[gianm]

IMO it's better in the main Druid repo, which would also help generate more discussion on the contents of the doc (there hasn't been much on the dev list from what I can see).

[gianm]

Could you add something about what these tags mean? I don't know what "Compatibility" means, for example. (How is it different from "Incompatible"?)

[gianm]

Also, my recollection is that in the thread about adding proposals, the consensus was that we should avoid doing PRs tagged with "Design Review" (instead, linking to a proposal is better, in an effort to separate the design-level discussion from the implementation-level discussion). So maybe that label should be removed or at least de-emphasized.

[leventov]

"Compatibility" is opposite of "Incompatible": it restores or ensures compatibility in some way. See this message in dev list.

I've started explaining tags in the "PR Management" section in the draft of a common checklist. Now I don't think this checklist should exist as a giant whole. Better to split it into smaller, more focused checklists.

"PR Management" section from the document above should be extracted as "PR action items checklist for a committer". (I don't plan to do this task myself in the very immediate future.)

[gianm]

IMO better to say ability instead of right. It sounds a bit more respectful to my ears.

[gianm]

I would prefer if this review checklist was removed from this PR and then potentially re-added separately from the act of creating a pull request template. This is because if we do end up having an official guide / checklist it should be in the main Druid repo, rather than a separate repo. It will make it easier to discuss its specific contents and evolve it over time in a way that reflects community consensus.

[gianm]

IMO it's better in the main Druid repo, which would also help generate more discussion on the contents of the doc (there hasn't been much on the dev list from what I can see).

[gianm]

There's a lot of overlap between this "Design" and the definition of a proposal: https://github.com/apache/incubator-druid/blob/master/.github/ISSUE_TEMPLATE/proposal.md. It's weird to have that template and this one talking about the same things from different angles. One way to address that is to take anything useful from this template that isn't already in the Proposal template, then merge that stuff into the Proposal template and simply reference that one here. And then open this template by saying that:

1. If you raised a proposal, just link to it.
2. If you didn't raise a proposal, think about doing that first, especially if the PR has any sort of 'interestingness' to its design that is worth discussing. If not, then include the same kind of information here in the PR.

I feel that encouraging people to raise proposals is useful because it promotes separation of design-level and implementation-level discussion. One point that was raised in the dev list when we talked about this is that commingling them can cause one to drown out the other.

[gianm]

This is similar to the "discuss other alternative solutions" guidance in the proposal template. However, the proposal template doesn't require it. I don't see a reason to require it. It might end up being artificial if forced.

[leventov]

@gianm A section "Design It Twice" in this book discusses this aspect and explains the benefits of trying to come up with an alternative, regardless of how obvious and undebatable the implemented design seems to be and how ridiculous the alternatives seem to be. It says that this excersize often helps to uncover important properties of the design and come up with a third design, better than the original one and the alternative.

[gianm]

I guess I can't speak to the argument in that book because I don't have a copy of it. But even though there are benefits to the thought experiment of thinking through alternate designs, I don't think we need to make it a hard requirement that people do it for every facet of every contribution. How about converting this to a suggestion rather than a requirement.

[leventov]

I suspect that even with this template, in some PRs design won't be discussed at all, because the first action of the contributor after opening the "New PR" interface on Github will be selecting all the template text and deleting it. People who contribute regularly will do the same (and that's ok) because they already read this template text once.

I don't worry that anybody will ever take this instruction too far. I'm sure that even with the current wording, contributors will still under-discuss the design rather than over-discuss it, on average. I think that softer wording will shift this even more towards under-discussion.

[gianm]

I don't think writing intentionally over-strict requirements is the right way to encourage good behavior. I think we should show a little more trust in contributors and write requirements that reflect what we actually expect from them (and from each other, since we are all contributors). It's certainly good to list out suggestions / things to consider that make a PR better, but phrasing them as requirements seems unwelcoming and disingenuous to my ears.

[jon-wei]

Always considering alternative designs is a good ideal, but in practice, If we had a hard requirement for the "describe alternatives" rule, I would expect many PRs to have empty fluff in that section or people just describing some awful alternative implementation to check off the requirement, I expect it would often not bring any real value if forced.

If the PR submitter shares in the spirit behind this rule, they would likely do if it's optional. If they don't, they'll ignore it or game the system even if it's a requirement.

If I saw an otherwise good PR that lacked this section, I would personally not be willing to reject it on that basis alone.

So I would agree with making this optional.

[jon-wei]

I also think making it a hard requirement shifts too much burden to the PR author.

If there are interesting alternative designs, a reviewer can bring them up during review.

If it's a PR where no reviewer can think of alternative designs that are worth considering, then I don't think it's reasonable to place that requirement on the PR author either.

[leventov]

I'll update to more optional language because I don't see a point in fighting for this.

[leventov]

Just answering:

I expect it would often not bring any real value if forced.

That's an intuitive expectation, but the counterintuitive point is that it actually brings real value far more often than expected.

If I saw an otherwise good PR that lacked this section, I would personally not be willing to reject it on that basis alone.

I don't think any problem with PR decoration is a basis for rejecting any PR at all.

I also think making it a hard requirement shifts too much burden to the PR author.

If there are interesting alternative designs, a reviewer can bring them up during review.

If it's a PR where no reviewer can think of alternative designs that are worth considering, then I don't think it's reasonable to place that requirement on the PR author either.

Authors are in a far better position to propose alternative designs. Reviewers often barely grasp the idea of the PR, and they only review the code on Github. Authors wrote the code, worked in IDE, understand all ins and outs.

In general, I think it's a good idea for PR authors to place as much burden as they can on themselves. Self-reviewing touches the same idea. Reasons:

• Self-reliance and removing the burden from colleagues is always a good idea. (Doing as much work as we can ourselves, collaborators will care of their work themselves. Not relying on collaborators or quality of their work.)
• Perhaps when we review our own code (and design decisions) we more likely learn something and improve our own style than when we review other people's code. The same phycological effect as behind "People don't learn from other's people mistakes, only their own." Or maybe because when we ourselves the source of "review comments" we don't tend to subconsciously reject them or treat them passive-aggressively.

But that's a whole other topic of discussion and I'm not going to mention anything of that in the template.

[jon-wei]

That's an intuitive expectation, but the counterintuitive point is that it actually brings real value far more often than expected.

That's fair, I don't think I would really know the effects of such a rule unless put in practice. But I think doing so is risky.

Some PRs are submitted by people with a pressing need to have that feature in Druid (e.g., their business relies on a certain feature they made), I don't expect complex PR submission requirements to hinder such people.

But as the PR submissions become more "casual", I think the likelihood of people being discouraged by complex requirements increases. I would personally rather put more burden on reviewers for ensuring quality than potentially hindering incoming contributions for this specific point.

I don't think any problem with PR decoration is a basis for rejecting any PR at all.

Hm, if I compare description of alternative designs to a description of the chosen design, I would be quite willing to hold off review on a PR or reject it until the author provides a description of what the PR is doing.

To the earlier point being discussed about enforcing the rule, I think it's true that making it a "hard requirement" could be psychologically useful, but that too I think has costs. As a general principle, I believe that having rules that aren't enforced consistently hurts the credibility of the body of rules as a whole and introduces some uncertainty.

[gianm]

I like the spirit of a list like this but I think we need to be careful with the wording to avoid being confusing to newish contributors. It seems to suggest that the list of things is required for every PR. In fact, it's more like a list of things to consider doing if appropriate, rather than a list of requirements. Not every change needs to be tested in a production environment. Not every change needs integration tests, and in fact it's probably counter-productive to try (for lots of changes, the slowdown from adding integration tests isn't worth it).

[leventov]

@jon-wei @gianm @clintropolis @jihoonson I've updated the PR template.

Regarding the concurrency checklist, I've created a two-level system with single line items + slightly extended (and linkable) descriptions. The checklist added to the Druid repository contains all one-liners which link to the general checklist, plus three Druid-specific items (see the bottom of the file). If it will be needed to diverge wording of Druid's items from the general checklist's items, the extended description can be copied over to the Druid's checklist file with adjustments, and the link from the one-liners list is updated to point to the copied description.

[jon-wei]

Hm, I'm still not entirely on board with the idea of the concurrency checklist having external links, but having an in-repo checklist with external references I think is better on balance than having only a link to an external checklist.

The individual items mentioned in the current in-repo checklist look okay to me from an initial review, and I guess longer term the intent is to develop the external checklist into a general set of guidelines commonly referenced by Java projects. I think I'm okay with running with this direction for now, and making adjustments if/when needed.

I had a few more comments, but otherwise LGTM.

[jon-wei]

The #daemon-threads and some other spots need https://github.com/code-review-checklists/java-concurrency in the link

[leventov]

There are three such links, they point to Druid specific items in the bottom of the page.

[jon-wei]

ah, got it

[jon-wei]

I think the wording on some of the checklist items could be made clearer, e.g.:

# RC.1. Aren’t ConcurrentHashMaps updated with multiple separate containsKey(), get(), put() and remove() calls instead of a single call to compute()/computeIfAbsent()/computeIfPresent()/replace()?

From that it's somewhat ambiguous as to which one is the positive recommendation, I think would be clearer if phrased as:

# RC.1. ConcurrentHashMaps should be updated with a single call to compute()/computeIfAbsent()/computeIfPresent()/replace() instead of with multiple separate containsKey(), get(), put() and remove() calls

and for places with similar use of "aren't".

[gianm]

A broad question: is the intent of this PR template to put our current practices into writing, or to define a new set of expected practices? I'm asking because there are a lot of things this template asks for that we do not currently always ask for on every PR.

If the intent is to put current practices into writing, I think we should make that clear by calling out what things are required and what are more like suggestions for gold-standard best practices (but, ultimately, not necessarily required).

If the intent is to define a new set of expected practices, that should also be made clear.

[himanshug]

this should take http://mail-archives.apache.org/mod_mbox/druid-dev/201904.mbox/browser into consideration, and we should conclude something on that thread.

I am not clear what are the "must haves" that would be enforced in each and every PR. Others should not be used to block a PR or else that is unfair to that PR's author.
My fear is that Long list of things will further the discrepancy among reviewers leading to some PRs getting stuck because they happened to be reviewed by certain reviewers vs others.

[leventov]

A broad question: is the intent of this PR template to put our current practices into writing, or to define a new set of expected practices? I'm asking because there are a lot of things this template asks for that we do not currently always ask for on every PR.

I think I added enough language to make everything optional. However, the expectation is that regular developers will do most of the things (which they often don't do now). So yes, this is an elevation of the standards.

The idea is that we think that doing these things is beneficial for the common productivity (when a PR author who is a regular contributor considers not only their own productivity right during the preparation of the given PR, but also productivity of the reviewers, and productivity of dozens of people who will work with their code later over the lifetime of the project). So we want to do these things rather than consider them as chores. The template is a memo for ourselves to not forget about them.

[leventov]

@gianm @himanshug do you have more comments?

[egor-ryashin]

I would rephrase "as when there are insufficient resources" or "when not enough resources are available".

[leventov]

Changed to "as when there are insufficient resources".

[leventov]

This PR has two approvals from committers. I'll merge it at the end of this week unless someone leaves more comments.

[himanshug]

so this got merged, however I don't see a consensus built as most druid reviewers/contributors did not approve it.. which means the discrepancy among the review comments would continue and #7206 (comment) is still valid.
I hope I am wrong .