I am coming from a long break and good to see community becoming bigger and people caring about ease of contribution.
Thanks Gian for starting this thread and I have had similar perceptions in the past. 1. Absolutely Agree on automating most style comments possible and try to get as close to 100% technically possible. As reviewers, whenever we provide a stylistic review comment, we should see if that could be automated. 2. Yes. There should be "must haves". here are my quick thoughts... - look for meaningful PR description containing what, why at least and how the change is tested. I usually give more weight to someone who is actually using that code and deploying it in their environment to test it. - user visible configuration changes and public APIs are documented - functional correctness - performant enough depending upon the context where it is used, not all code demands to be lock-less or doing the cool tricks :) - good enough memory usage/retention. it should be something that doesn't bring Druid process down but also this shouldn't give reviewer free pass on criticizing every allocation or not doing things in most optimal way leading to philosophical/research discussions. code simplicity and not using deepest java tricks is OK in most cases. - if the change is visible to end user then more care should be taken to ensure backward compatibility, rolling upgrades etc. Exceptions are made here but we should be aware of those and PR should be tagged so that we put right blurb in the release notes. - there are multiple designs/ways to solve one problem which are often equally good, so ask for changes if changed design/way is concretely better in that context. Also, for bigger changes, I have seen suggestions on doing design reviews upfront before the code but my experience is that only high level designs are apparent before coding it 70-80%. so code would invariably have design components to it. - I am glad I don't see too many comments on unit testing these days but sometimes reviewers try to push for "good" coverage and covering all the edge cases in unit tests. I personally favor writing enough unit tests that gives me confidence that my code works, more tests are nice but not have same level of ROI. That said, it is very difficult to have 100% concrete rules covering everything, we can only strive to minimize subjectivity. We must not have comments based on our preference and keep our comments similar in scope as other reviewers based upon agreed upon must haves. reviewer should also introspect , "what is the cost/impact of not having the comment addressed and how difficult will it be to make that change later when/if it becomes a necessity?". If the cost/impact of not getting the comment addressed is low and no clear major benefit, reviewer should self-resolve the conflict in favor of contributor. 3. Agree. Also, it would be somewhat taken care of if scope of review comments was limited to must haves as much possible. 4. Yes. That said, I think this is also because of gamed github/git credit system. A person gets somewhat formal credits for writing code whereas understanding someone else's code and providing meaning review comments(that belong to the "must haves" category) is significant amount of work. There is also work done by person providing good design inputs that matter. But, those don't get any formal credits. This wouldn't apply to committers, but most open source projects only look at code contribution as metric of contribution so any new person has to find reasons to write code. There are many non-committer Druid users who spend time understanding Druid source code to fix their own problems. They could be great reviewers but have no motivation to do so. On Big vs Small PR: Due to small PR getting more scrutiny, chain of small PRs would take way too much work/time on author's part to get merged so He/She would prefer all in one PR. Also, sometimes it is ok.. - introducing a new extension - a major change that is hard to break into small PRs but is totally opt-in - its just way simpler to test/benchmark the thing and get things done in that unit if we do make a rule about large PRs then it must be followed by all PRs so that it doesn't become biased for specific contributions. And, unfortunately such a rule would hurt people making most meaningful contributions to the project because, most often, large changes are doing something very useful for the project. Fears of code becoming unmaintainable and losing pace of innovation are somewhat real but heavily subjective which make code review comments very very inconsistent among reviewers, really lead to the "hoop-jumping". This specially hurts PRs that have attention by 1 or few reviewers because PR would just linger on till contributor submits to the preferences of reviewer. Focussing on "must haves" is important to minimize the diversity in review comments based on reviewer. I am not really afraid of tech debt and can sacrifice some of it in favor of other things specially because of subjectivity associated. If Druid has wide adoption, there are lots of people who know the codebase, actively contribute to it, use the project and hence care that it continues to work/perform ... innovation and features would continue to happen(as they have been in many years in the past) and tech debt would be taken care of, at least in parts of code that get actively modified due to the need of regular change.
