[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-05-31 21:14:29
Message-ID: 39b9b81e-4aab-37ec-6b48-b7fa202446b9 () oracle ! com
[Download RAW message or body]



On 5/24/2018 9:31 AM, Nir Lisker wrote:
> Thanks for the detailed plan Kevin,
>
>     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.
>
>
> It will be a good idea to list the reviewers/experts (names and mail) 
> according to their field, as done in the outdated Wiki [1]. This way 
> contributors know who to address in a review request mail. Currently, 
> I need to filter a subcomponent in JIRA and see who provides the fixes 
> there to know who to ask.

That is a good idea. Reviving this page after we have the initial list 
of reviewers seems like a good idea.


>     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.
>
>
> Continuing the point above, it makes sense to me that a Reviewer role 
> is assigned per field (whether a "field" is module, a set of packages 
> or something else). While formally we need to give a list of Reviewers 
> for OpenJFX, practically I don't think a Reviewer who contributed 40 
> out of his 42 commits to javafx.base can (and probably wouldn't 
> anyway) Review in javafx.web. What I'm getting at is that whatever the 
> threshold numbers are, it makes sense, at least informally, to count 
> per field. If I submit 5 contributions per module and pass the 
> threshold, what exactly am I qualified to Review?


That's an interesting, and accurate, observation. It sort of goes along 
with your earlier point about having areas of expertise.


> Granted, the threshold numbers includes familiarizing oneself with 
> code patterns and tools which are global to the project, so to become 
> a Reviewer in a 2nd field takes much less effort than the first time.
>
> This is just a point I wanted to make about the Reviewer role. We 
> don't have to change formal policies.
>
>     I propose that a New Feature, API addition, or behavioral change
>     must be reviewed / approved by a "lead". 
>
>
> Can you give the guidelines by which a lead reviews / approves one of 
> the above?

Ultimately it will be a judgment call. I can't speak for Johan, but what 
I usually do is see whether the proposed feature is a good fit for the 
API, doesn't raise any compatibility concerns, is supportable, testable, 
etc., and then get to the more detailed review of the spec and API 
additions themselves.

>     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.
>
>
> I would hope that the people in openjfx-dev who are not watching all 
> PRs will have a chance to comment before the work starts, and not when 
> the work is done and before it's merged. Visiting the mirror from time 
> to time reveals to me PRs that weren't mentioned on the mailing list. 
> Those PRs might be in conflict with work that isn't visible on GitHub. 
> Iv'e brought it up in a past discussion about the mirror - we need to 
> centralize the discussion medium. It's the normal price to pay for 
> synchronization.
>

A fair point, which is why I proposed that one of the requirements for 
having this fast track review includes "... sending email to openjfx-dev 
when you first make a PR that you intend to have merged into the 
develop" so that those who are interested in a particular bug will know 
that a fix is under review.

Thanks.

-- Kevin


> - Nir
>
> [1] https://wiki.openjdk.java.net/display/OpenJFX/Code+Ownership 
> <https://wiki.openjdk.java.net/display/OpenJFX/Code+Ownership>
>
> On Thu, May 24, 2018 at 1:16 AM, Kevin Rushforth 
> <kevin.rushforth@oracle.com <mailto:kevin.rushforth@oracle.com>> 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
>     <http://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
>     <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
>     <http://mail.openjdk.java.net/pipermail/openjfx-dev/2018-February/021335.html>
>
>     [3] https://wiki.openjdk.java.net/display/csr/Main
>     <https://wiki.openjdk.java.net/display/csr/Main>
>
>

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