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
