RFR (S): 8217666: gc/nvdimm/* should not be included any tiers
leo.korinth at oracle.com
Mon Jan 28 19:31:26 UTC 2019
On 28/01/2019 20:09, sangheon.kim at oracle.com wrote:
> Hi Leo,
> Thank you for reviewing this.
> On 1/28/19 2:25 AM, Leo Korinth wrote:
>> Hi Sangheon,
>> a few questions on the webrev:
>> 1) what does the added vm.nvdimm.test.enabled line in TEST.ROOT do?
> vm.nvdimm.test.enabled is added at the requires.properties list at
> TEST.ROOT and this is necessary for its work flow.
> FYI, without that change, we will see 'Syntax error in @requires
> expression: invalid name: vm.nvdimm.test.enabled'.
Thank you for the explanation.
>> 2) most of the *Enabled() functions in VMProps.java read properties,
>> but here we read the environment, why is that?
> I think the env. variable approach seems clearer to use considering
> those tests will be tested on limited situations.
> JTREG and VM which runs requires.VMProps will be affected.
> JTREG doesn't propagate env variables to JDK under test unless they are
> specified via -e flag.
> I considered adding a property as well, but I ended up with the
> environment variable.
Okay, I just wanted to know that it was not by accident.
>> 3) maybe the property should be "vm.gc" prefixed instead of "vm"
>> prefixed, maybe not. What do you think?
> I'm okay with 'vm.gc' prefix.
> 'vm.gc.nvdimm.test.enabled' unless others dislike it. :) > Let me post the updated webrev after getting more comments.
I am fine with either. Your change looks good to me, and thanks a lot
for fixing this problem. Keep in mind that I am just a commiter.
>> Thanks for disabling these test cases!
>> On 26/01/2019 16:35, sangheon.kim at oracle.com wrote:
>>> Hi all,
>>> Can I have reviews that excludes gc/nvdimm jtreg tests?
>>> Those tests were introduced by JDK-8202286 (Allocation of old
>>> generation of Java heap on alternate memory devices) and tried to
>>> exclude all tests from all tiers. But it was incomplete so one of the
>>> tests failed and JDK-8217406 (gc/nvdimm/TestOldObjectsOnNvdimm.java
>>> failure) was filed recently.
>>> The patch includes to exclude gc/nvdimm from TEST.groups,
>>> hotspot_misc group(which is the reason why JDK-8217406 occurred). In
>>> addition, added @requires to gc/nvdimm tests to avoid running the tests.
>>> CR: https://bugs.openjdk.java.net/browse/JDK-8217666
>>> webrev: http://cr.openjdk.java.net/~sangheki/8217666/webrev.0/
>>> Testing: manual tests w/, w/o VM_NVDIMM_TEST environmental variable.
More information about the hotspot-gc-dev