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.rushfo...@oracle.com <mailto:kevin.rushfo...@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>
