On Tue, 9 Jan 2024 14:32:19 GMT, Pavel Rappo <pra...@openjdk.org> wrote:
> I have a few questions on this PR. Firstly, general: how come this PR hasn't > been reviewed by jshell people? I appreciate that the change is about GCs, > but generally a PR should be reviewed by experts in all areas that that PR > touches. There are multiple reasons why I thought that it is fine to integrate without further reviews, some of them are: * This is a test change that only modifies the runtime environment of the test to allow GCs to handle the load. This has been evaluated and verified that the live data set is too large for some collectors to handle. I.e. the PR changes no jshell/javadoc or test code at all. * The problem is well understood - too much live data used by the test due to other changes. There did not seem to be a memory leak or something either while looking at the logs (I did not mention this in the description, but I checked). * The CR has also explicitly been moved to the GC team to handle (from the javadoc component). * The duplicate CR [JDK-8318025](https://bugs.openjdk.org/browse/JDK-8318025) is three months old now, with an explicit comment by experts in that area that this is likely a heap capacity issue, which the evaluation confirmed. * The risk that the fix is wrong or worsens the situation (new test failures) is minimal imho. * Formally the 24h rule so that everyone can participate has been observed. It is unfortunate that the "kulla" label has not been applied which caused some people to miss the PR (apparently this label explicitly notifies you(?) jshell experts), so apologies for missing that. However this (and the integration due to above reasons) does not mean that the discussion about the change is done with this integration, and obviously it can still be commented on, re-evaluated and changed as needed. ------------- PR Comment: https://git.openjdk.org/jdk/pull/17304#issuecomment-1883345570
