[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: OpenJFX code review policies, etc.
From:       Kevin Rushforth <kevin.rushforth () oracle ! com>
Date:       2018-06-12 21:10:11
Message-ID: e5aa5f8c-7397-8eb4-1693-8a9794d2afc7 () oracle ! com
[Download RAW message or body]

Yes, that is an important clarification of the new policy. Thanks.

-- Kevin


On 6/12/2018 1:55 PM, Phil Race wrote:
> One other point .. something I mentioned off line to Kevin but he did 
> not so far bring up here,
> is that when counting the number of reviewers on a fix, we must 
> require at least one Reviewer,
> with a capital "R", but to make up the total of 2 reviewers, it is 
> sufficient to have one other person
> who only has committer status .. or even author status (maybe), be the 
> second reviewer.
> We've used this policy on the JDK AWT/2D/Swing stack for a long time. 
> It not only frees up the
> scarce "R"eviewers, but it helps train new ones :-), whilst still 
> ensuring at least two sets of eye balls
> were on it.
>
> -phil.
>
> On 05/24/2018 10:36 AM, Kevin Rushforth wrote:
>> Phil pointed out one glaring typo in the summary and also a couple 
>> things in the details that could be clarified.
>>
>>> The short version of the proposal is:
>>> ...
>>> 2. Revised code review policies for different types of changes: 
>>> simple, low-impact fixes (1 Reviewer); higher-impact fixes (2 
>>> Reviewers + allow time for others to chime in); Features / API 
>>> changes (CSR approval, including approval by a "lead", plus 3 
>>> Reviewers for the code)
>>>
>>
>> That last been should be: CSR approval, including approval by a 
>> "lead", plus *2* Reviewers for the code. I had it right in the 
>> details, but made a typo in the short version. While some reviews 
>> might have more than 2, it was certainly not my intent to mandate it.
>>
>>> A. Low-impact bug fixes. ...
>>> One reviewer is sufficient to accept such changes. As a courtesy, 
>>> and to avoid changes which later might need to be backed out, if you 
>>> think there might be some concern or objection to the change, please 
>>> give sufficient time for folks who might be in other time zones the 
>>> chance to take a look. This should be left up to the judgment of the 
>>> reviewer who approves it as well as the contributor.
>>
>> To clarify, the intent is to avoid pushing changes that might be 
>> controversial, and not to mandate unnecessary delay for truly simple 
>> fixes (e.g., fixing a build break). Reviewers and Committers are 
>> expected to use their best judgment here.
>>
>>> C. New features / API additions. This includes behavioral changes, 
>>> additions to the fxml or css spec, etc.
>>>
>>> ... a New Feature, API addition, or behavioral change must be 
>>> reviewed / approved by a "lead". Currently this is either myself or 
>>> Johan Vos as indicated above.
>>>
>>> I also propose that we continue to use the CSR process [3] to track 
>>> such changes. The CSR chair has indicated that he is willing to 
>>> track JavaFX compatibility changes even though FX is no longer a 
>>> part of the JDK.
>>
>> The "approval by lead" means approving the API / feature change via 
>> the CSR. A "lead" often will be one of the code reviewers as well, 
>> but need not be as long as they approve the API change itself via the 
>> CSR.
>>
>> -- Kevin
>>
>>
>> On 5/23/2018 3:16 PM, Kevin Rushforth wrote:
>>> To: OpenJFX Developers
>>>
>>> As I mentioned in a message last week [1] I would like to restart 
>>> the discussion we started a few months ago [2] around making it 
>>> easier to contribute code to OpenJFX. To this end, I like to make 
>>> some concrete proposals around code review / API review policies.
>>>
>>> Before getting to the details, I would like to acknowledge Gluon's 
>>> contributions to the OpenJFX project, specifically those of Johan 
>>> Vos. I am pleased to announce an expanded role for Johan Vos in the 
>>> OpenJFX project. I would like to announce that starting now, Johan 
>>> is effectively a co-lead for the purposes of setting direction, and 
>>> approving new features for the Project.
>>>
>>> The short version of the proposal is:
>>>
>>> 1. Formalize the concept of Reviewers with an initial list of 
>>> Reviewers and a defined criteria for adding additional Reviewers.
>>>
>>> 2. Revised code review policies for different types of changes: 
>>> simple, low-impact fixes (1 Reviewer); higher-impact fixes (2 
>>> Reviewers + allow time for others to chime in); Features / API 
>>> changes (CSR approval, including approval by a "lead", plus 3 
>>> Reviewers for the code)
>>>
>>> 3. Streamlining reviews of changes developed in the GitHub sandbox: 
>>> provided that the review policy is followed to before a PR is merged 
>>> into the develop branch in GitHub, a fast-track review can happen 
>>> pointing to the changeset that was merged and the PR, which has 
>>> review comments.
>>>
>>> Details follow.
>>>
>>> Quoting from my earlier message:
>>>
>>>> "Code reviews are important to maintain high-quality contributions, 
>>>> but we recognize that not every type of change needs the same level 
>>>> of review. Without lowering our standards of quality, we want to 
>>>> make it easier to get low-impact changes (simple bug fixes) accepted."
>>>
>>> To that end, I propose the following policies. Many of these will 
>>> involve judgment calls, especially when it comes to deciding whether 
>>> a fix is low impact vs. high-impact. I think that's OK. It doesn't 
>>> have to be perfect.
>>>
>>> Recommendations
>>>
>>> 1. I recommend that we formalize the concept of Reviewers, using the 
>>> OpenJDK Reviewer role for the OpenJFX Project.
>>>
>>> A. I will provide an initial list of reviewers to the registrar 
>>> based on past contributions, and also recognizing Committers who 
>>> have become experts in their area. This is the only time we will 
>>> have such latitude as the OpenJDK Bylaws specify the policy we need 
>>> to follow for nominating and voting upon additional Reviewers.
>>>
>>> B. We need to set formal guidelines for becoming a Reviewer. The JDK 
>>> uses a threshold of 32 significant contributions. While we don't 
>>> want to relax it too much, one thing I have been discussing 
>>> informally with a few people is that a Committer with, say, 24 
>>> commits, who regularly participates in reviews, offering good 
>>> feedback, might be just a good a reviewer (maybe even better) than 
>>> someone with 32 commits who rarely, if ever, provides feedback on 
>>> proposed bug fixes. I'm open for suggestions here.
>>>
>>> One thing I'd like to add is that we expect Reviewers to feel 
>>> responsible not just for their piece, but for the quality of the 
>>> JavaFX library as a whole. I might work with some folks at Gluon and 
>>> here at Oracle to draft a set of expectations for reviewers.
>>>
>>>
>>> 2. Code review policies
>>>
>>> All code reviews must be posted on the openjfx-dev mailing list -- 
>>> even simple fixes. I propose that we have the following code review 
>>> policies for different types of changes. I also note that if there 
>>> is disagreement as to whether a fix is low-impact or high-impact, 
>>> then it is considered high-impact. In other words we will always err 
>>> on the side of quality by "rounding up" to the next higher category. 
>>> The contributor can say whether they think something is low-impact 
>>> or high-impact, but It is up to a Reviewer to initially decide this.
>>>
>>> A. Low-impact bug fixes. These are typically isolated bug fixes with 
>>> little or no impact beyond fixing the bug in question; included in 
>>> this category are test fixes (including new tests), doc fixes, and 
>>> fixes to sample applications (including new samples).
>>>
>>> One reviewer is sufficient to accept such changes. As a courtesy, 
>>> and to avoid changes which later might need to be backed out, if you 
>>> think there might be some concern or objection to the change, please 
>>> give sufficient time for folks who might be in other time zones the 
>>> chance to take a look. This should be left up to the judgment of the 
>>> reviewer who approves it as well as the contributor.
>>>
>>> B. Higher impact bug fixes or RFEs. These include changes to the 
>>> implementation that potentially have a performance or behavioral 
>>> impact, or are otherwise broad in scope. Some larger bug fixes will 
>>> fall into this category, as will fixes in high-risk areas (e.g., CSS).
>>>
>>> Two reviewers must approve to accept such changes. Additionally, the 
>>> review should allow sufficient time for folks who might be in other 
>>> time zones the chance to review if they have concerns.
>>>
>>> C. New features / API additions. This includes behavioral changes, 
>>> additions to the fxml or css spec, etc.
>>>
>>> Feature requests come with a responsibility beyond just saying "here 
>>> is the code for this cool new feature, please take it". There are 
>>> many factors to consider for even small features. Larger features 
>>> will need a significant contribution in terms of API design, coding, 
>>> testing, maintainability, etc.
>>>
>>> To ensure that new features are consistent with the rest of the API 
>>> and the desired direction of the Project, I propose that a New 
>>> Feature, API addition, or behavioral change must be reviewed / 
>>> approved by a "lead". Currently this is either myself or Johan Vos 
>>> as indicated above.
>>>
>>> I also propose that we continue to use the CSR process [3] to track 
>>> such changes. The CSR chair has indicated that he is willing to 
>>> track JavaFX compatibility changes even though FX is no longer a 
>>> part of the JDK.
>>>
>>> For the review of the implementation, I propose that we use the same 
>>> "two reviewer" standard for the code changes as category B.
>>>
>>>
>>> 3. Streamlining the review process for changes developed on GitHub
>>>
>>> A fix that was developed as pull-requests (PRs) on GitHub is 
>>> eligible for a fast-track review, if:
>>>
>>> A. The PR was squashed / merged into the develop branch as a single 
>>> changeset
>>> B. No follow-on changesets were merged into develop as part of that 
>>> same fix
>>> C. The changeset is "whitespace clean" -- meaning that you have run 
>>> 'tools/scripts/checkWhiteSpace' on the final changeset (we might 
>>> want to add this to the CI build).
>>> and
>>> D. All code review policies outlined above in #2 were followed prior 
>>> to the PR being approved and merged into the develop branch on 
>>> GitHub. This includes sending email to openjfx-dev when you first 
>>> make a PR that you intend to have merged into the develop branch to 
>>> give other reviewers who may not be watching all PRs a chance to 
>>> comment before it is merged.
>>>
>>> A "fast-track" review is a quick sanity check before the change is 
>>> committed and pushed to the jfx-dev repo on hg.openjdk.java.net. 
>>> This fast track review just needs to point to the GitHub changeset 
>>> that was merged and to the PR, which will have any review comments. 
>>> If there are no compelling reasons why the PR can't be pushed to 
>>> jfx-dev, then it can be pushed.
>>>
>>>
>>> Please let me know your thoughts on the above proposals.
>>>
>>> Thank you all for being a part of this community.
>>>
>>> -- Kevin Rushforth, OpenJFX Project Lead
>>>
>>> [1] 
>>> http://mail.openjdk.java.net/pipermail/openjfx-dev/2018-May/021867.html
>>>
>>> [2] 
>>> http://mail.openjdk.java.net/pipermail/openjfx-dev/2018-February/021335.html
>>>
>>> [3] https://wiki.openjdk.java.net/display/csr/Main
>>>
>>
>

[prev in list] [next in list] [prev in thread] [next in thread]