Re: OpenJFX code review policies, etc.
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
Re: OpenJFX code review policies, etc.
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
Milestone dates for OpenJFX 11 release
We are getting close to the start of rampdown for JDK 11 [1]. Now that FX is no longer bundled we don't need to follow exactly the same schedule of milestones, although we do want to release this first unbundled FX 11 release at the same time JDK 11 ships (or slightly sooner). Here is the proposed rampdown schedule for FX 11. The main differences between the JDK schedule and the FX schedule is that we don't need as long a back end as a full JDK feature release 2018-07-09 : RDP1 (aka feature freeze) 2018-08-06 : RDP2 2018-08-27 : freeze for GAC build 2018-09-18 : GA I am proposing a shorter time after RDP2 than the JDK because I don't think we need the long back end of a full JDK feature release (not as many moving parts or stakeholders). I will send out the proposed ramp-down policies, but as a starting point I imagine we want something similar to what the JDK uses with a couple modifications: 1. Since this is our first unbundled release and there are likely to be plenty of bugs that need fixing during RDP1, so I propose to postpone forking the repo until RDP1. This means a 4 week downtime where there is no place to push new features / enhancements unless they are critical to openjfx 11 (in which case they will be an exception). I think this is a reasonable trade-off for this release 2. I don't plan to propose any restrictions on P4 bugs before RDP2. The focus should be on more serious bugs, and we likely won't fix many P4s, but if a safe P4 bug fix is proposed, reviewed, and tested, then I see no reason not to take it between RDP1 and RDP2. -- Kevin [1] http://openjdk.java.net/projects/jdk/11/
