On Fri, 25 Apr 2025 16:27:13 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> Kevin Walls has updated the pull request incrementally with one additional >> commit since the last revision: >> >> REMOVE_TIMEOUT_SECONDS > > test/jdk/sun/jvmstat/monitor/MonitoredVm/MonitorVmStartTerminate.java line 78: > >> 76: private static final int PROCESS_COUNT = 10; >> 77: private static final int ARGS_ATTEMPTS = 5; >> 78: private static final int REMOVE_TIMEOUT_SECONDS = 300; > > I'm still not convinced we need a large timeout here. The default timeout is > 120 seconds, so jtreg will time out the test before waitForRemoval() times > out itself. Yes -- Maybe just take out this extra timeout. I'm being too respectful of things that have been in the test forever, but aren't of much use. Some tests have their own timeouts, when jtreg can just time them out. Checking again, the timeouts I've seen from this tests are from the jtreg harness, not the loop that had a 1000 second timeout, or even when I change it, it's still jtreg giving the timeout. Should remove it. ARGS_ATTEMPTS could be even longer. If it completely fails to read a process arguments, we are at risk of failing later due to timeout. We may as well try harder, this is the part that might fail and need retrying when the processes are slow to start. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24843#discussion_r2062720323
