Draft Public Code Review Proposal

[Poonam Bajaj]

Hi Dalibor,

On 8/3/2011 8:33 AM, Dalibor Topic wrote:
> On 7/14/11 7:00 AM, David Holmes wrote:
>   
>> Hi Dalibor,
>>
>> On 14/07/2011 11:45 AM, Dalibor Topic wrote:
>>     
>>> Next item on my draft list is code review. The proposal below is inspired by the developer guide section on code review, and in part inspired by the processes in OpenJDK 6. It tries to encourage transparency, and getting more eyes on the code, while at the same time allowing established practices, like asking component leads for additional reviews, to continue to work.
>>>
>>> Public code review:
>>>
>>> Preamble: The new infrastructure is not ready yet, so we won't switch to it for the next release in OpenJDK (i.e. 7u2, as 7u1 is a CPU) just yet. The process here may change for 7u4.
>>>
>>> Pre-requisite: Before asking for code review, make sure your fix is applied on the most recent revision of the code you intend to work on, that there are no build errors on the supported platforms for the release you're working on, and that any included or otherwise applicable tests pass without failures on all relevant platforms. If you're not able to build or test on all supported platforms, you MUST let the reviewers know which ones you were able to build and test on, as well as whether you anticipate any issues on the platforms you haven't been able to build and test on.
>>>
>>> Rule 0: Code reviews for public JDK 7 Update forests MUST be done publicly: Either through e-mail on jdk7u-dev at openjdk.java.net mailing list, or using some other suitable public mechanism. If a code review is not done on the jdk7u-dev at openjdk.java.net mailing list, as part of the approval request for inclusion of the fix into a public JDK 7 Updates forest, which you MUST send to the jdk7u-dev at openjdk.java.net mailing list, a URL for the public code review MUST be provided.
>>>       
>> At present who comprises the set of authors, commiters and reviewers for the jdk7u project?
>>     
>
> The initial set of committers is the set of individuals who have committed code into JDK 7,
> i.e. the output of 
>
> for i in . corba/ hotspot/ jaxp jaxws/ jdk/ langtools/ ; do hg -R $i log --template "{author}\n" ; done | sort | uniq 
>
> on jdk7.
>
> The initial set of reviewers is the set of individuals who have reviewed code committed into JDK 7, i.e. are mentioned in a Reviewed-by: line in a commit.
> I don't have a nice one liner for that yet, though.
>
> The initial set of authors is the set of individuals who have submitted code into JDK 7, without having committed it themselves,
> i.e. the individuals mentioned in Contributed-by lines who are not Committers. Again, no sweet one liner for that either,
> contributions welcome.
>
> The rationale for going with that broad set of individuals is that if you helped create or review changesets for JDK 7, you can now 
> help in the same roles with the updates. 
>
>   
Can we have a page listing the names of authors, commiters and reviewers 
for jdk7u project ?

Thanks,
Poonam

>>> Rule 1: If the content of a changeset submitted for review for a public JDK 7 Update forest is the same as the corresponding changeset submitted or committed for JDK 8, then it does not have to be resubmitted. In that case its URL will suffice.
>>>       
>> I don't understand what you mean by resubmitted. Do you just mean that we can give the URL to the original JDK8 webrev? 
>>     
>
> Yes. Or the changeset, in case that it has been committed to JDK 8.
>
>   
>> I assume review is still necessary.
>>     
>
> Yes.
>
>   
>>> Rule 2: If the content of a changeset submitted for review for a public JDK 7 Update forest differs from the corresponding JDK 8 changeset, or if there is no corresponding JDK 8 changeset, then the changeset MUST be submitted publicly for review:
>>>
>>>      * A very small changeset MAY be submitted as a simple patch, as described in http://openjdk.java.net/contribute/
>>>
>>>      * Other than that, changesets SHOULD be submitted as webrevs on the code review server, cr.openjdk.java.net.
>>>
>>>      * Alternatively, a URL for the webrev of the changeset MAY be provided
>>>
>>> Rule 3: A changeset submitted for the JDK 7 Update mainline forest (jdk7u) MUST be reviewed by at least one reviewer. The maintainer responsible for approval of the changeset MAY request additional, specific reviewers to review the changeset, e. g. component leads.
>>>
>>> Rule 4: A changeset submitted for a JDK 7 Update release forest (like jdk7u2, once it's branched off) MUST be reviewed by at least two reviewers. The maintainer responsible for approval of the changeset MAY request additional, specific reviewers to review the changeset, e. g. component leads.
>>>       
>> So I don't understand how rules 3 and 4 interact. Let's assume that jdk7u exists and we haven't yet forked off jdk7u2. I have a fix that must be in 7u2 so I push to the jdk7u-dev repo. According to the above this requires a single reviewer. When 7u2 repos are created I assume they will contain the current contains of 7u mainline - is that right? 
>>     
>
> Yes.
>
> Is it after that point that commits to 7u2 require two reviews (presumably because by that stage we anticipate 7u2 is fairly close and so we need to be extra careful when doing commits)?
>
> Yes.
>
>   
>> I assume that the approval of the Project and Technical leads are in addition to reviews themselves.
>>     
>
> Yes.
>
>   
>> What is the process for obtaining approval?
>>     
>
> Send an e-mail to jdk7u-dev at openjdk, with
>
> Subject: Request for approval for CR $NR
>
> With the body containing 
>
> a) a link to the publicly visible bug on the bugs.sun.com site (or its equivalent), or a description of the change in case no publicly visible bug is here
> b) a link to the publicly visible webrev or changeset (in case it's in JDK 8 already)
> c) if the review is taking place somewhere else, a link to the public review thread
> d) if the fix has already been reviewed for inclusion into jdk7u-dev, the list of reviewers
> e) once we branch off 7u2, you'll also need to add the forest the fix is targeted for
>
>   
>> I would expect that approval, in principle at least, for a given CR needs to be given upfront, and then again once the final code change is ready.
>>     
>
> I think that approval up front + code review should be sufficient. That may change for phase 2 of a release.
>
>   
>> What are the expectations and limitations on placing things into an update release?
>>     
>
> Proposed changes for the OpenJDK jdk7u-dev forest should fit into one of the following buckets:
>
> * Regressions from 7 GA
> * Significant issues raised through OpenJDK or otherwise
> * Fixes for crashes, hangs, corruption etc.
> * Fixes required to make new JDK 7 functionality usable
> * Low risk fixes with automated regression tests
> * Approved requirements for a specific Oracle release
>
>   
>> FYI I have a fix (7039182) that is in 8 and needs to be in 7u2 and I'd like to move on this asap, so don't mind being the initial tester of the new process, as it were.
>>     
>
> Sorry for the delay, and thanks for volunteering. Edvard will be your host while I'm away. ;)
>
> cheers,
> dalibor topic
>