[11u] Sconpe of Review ... was RFR(S): 8241234: Unify monitor enter/exit runtime entries.

[Andrew Dinn]

On 09/09/2020 16:49, Lindenmaier, Goetz wrote:
> It is pointless to ask reviewers to judge the risk
> because reviews are only done if the change had to be adapted.
> There are complex changes that just apply clean and thus are
> downported without review.  

I think you have very much overstated your case in that opening line. If
a patch needs adapting in anything other than a trivial manner then it
cannot be a bad thing for the reviewer to consider the risk involved.
That benefit cannot be removed by the fact that some changes bypass the
review process.

It might be diminished, of course. But by the same token, if  a complex
patch does not need adapting then it might still be a good thing for the
maintainer who is going to approve or disapprove it to consider the
risk. Indeed, I hope that this consideration is foremost in the
maintainers' minds.

Either way, it's far from /pointless/ for reviewers or maintainers to
consider risks and where they think it important negotiate the details
with the downporter. Likewise if the downporter is unsure then they
really ought to initiate such a dialogue. Such actiuon might well be
redundant in many cases but I am sure those will be easy to spot.

> Judging the risk is clearly a thing of the downporter. This is formulated 
> in Rule 1 of Oracles's Updates 
> description : http://openjdk.java.net/projects/jdk-updates/approval.html ,
> and also mentioned in step 6 of jdk11us description: 
> https://wiki.openjdk.java.net/display/JDKUpdates/How+to+contribute+a+fix
> This is supposed to help the maintainer to decide about the risk.

That would be fine if every downporter was happy and able to make the
assessment. However, there are definitely going to be cases where that
is not so.

I'm not sure why you are stating the requirements here in such absolute
terms. Note that in my previous post I did not recommend that /all/
responsibility be shifted to the reviewer or maintainer. I recommended
that reviewers consider helping downporters with complex backports,
including those that might benefit from trimming or re-implementing a fix.

> The major task of the review is to make sure the downported change 
> is correct, i.e. has the same effect on 11 as the original one.
> Nevertheless, if there is a  reviewer and he feels bad about a 
> change, he should communicate his concerns! 
I think it is misguided to separate the decision as to correctness from
concerns over the risk associated with a change. My experience is that
cases where it is hard to foresee all the consequences of downporting a
change are far from rare. For a lot of complex changes -- and even some
simple ones -- a 100% foolproof assessment of correctness is never going
to be available. In such cases the risk of breaking something really
ought to be factored in by the reviewer because s/he can only speak to
the /likely/ correctness (as opposed to /absolute/ correctness) of the
patch.

regards,


Andrew Dinn
-----------
Red Hat Distinguished Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill