On Fri, 20 Oct 2023 13:28:19 GMT, Kevin Walls <[email protected]> wrote:
>> From studying test failures, it looks like the way the test identifies its
>> related processes is failing.
>> It checks the mainArgs of a process by attaching, and looks like it
>> occasionally misses getting a valid match. The hasMainArgs method ignores
>> exceptions as it is expecting some exceptions: it is going to test unrelated
>> java process which happen to start.
>>
>> It should retry this main args check on failure, but not too many times to
>> be a burden on other valid unrelated processes, and should also log the PIDs
>> that have an issue so we can see if this is part of any future failure.
>>
>> Other small logging changes so we can see more easily the progress through
>> the test.
>
> Kevin Walls has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains six additional
> commits since the last revision:
>
> - Check main args fetching was ok. Increase sleep.
> - nits
> - Merge remote-tracking branch 'upstream/master' into MonitorVmStartTerminate
> - nit1
> - Comment, and move takeNap() sleep method.
> - 8209595: MonitorVmStartTerminate.java timed out
Looks good.
Placed a couple of nits.
Thanks,
Serguei
test/jdk/sun/jvmstat/monitor/MonitoredVm/MonitorVmStartTerminate.java line 183:
> 181: // As we have seen test timeouts due to missing a
> notification,
> 182: // we should retry this attempt to check arguments for a
> match.
> 183: for (int i=0; i<ARGS_ATTEMPTS; i++) {
Nit: Need spaces around `=` and `<`.
test/jdk/sun/jvmstat/monitor/MonitoredVm/MonitorVmStartTerminate.java line 199:
> 197: System.out.println("hasMainArgs(" + id + "): " + e);
> 198: }
> 199: takeNap();
Nit: I'm thinking if moving this after line 197 would be more clear.
-------------
Marked as reviewed by sspitsyn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16077#pullrequestreview-1665950516
PR Review Comment: https://git.openjdk.org/jdk/pull/16077#discussion_r1351289715
PR Review Comment: https://git.openjdk.org/jdk/pull/16077#discussion_r1351291242
