RFR: 7903580: Allow for re-attempting agent creation when an attempt fails [v2]

[Jaikiran Pai]

On Mon, 13 Nov 2023 18:30:04 GMT, Jonathan Gibbons <jjg at openjdk.org> wrote:

>> Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   update the tests to match the log message change
>
> src/share/classes/com/sun/javatest/regtest/exec/Agent.java line 101:
> 
>> 99:         try {
>> 100:             pidMethod = Process.class.getDeclaredMethod("pid"); // only available in Java 9+
>> 101:         } catch (NoSuchMethodException | SecurityException e) {
> 
> It can throw `UnsupportedOperationException`
> https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/Process.html#pid()

Good catch. Given that the reason I introduced the pid() call was for better debuggability, I have now updated to PR to catch the broad `Exception` type, both at the place where we reflectively check for the presence of `pid` method as well as where we reflectively call the `pid()` method, to ignore any exceptions and return `-1` in such cases.

> src/share/classes/com/sun/javatest/regtest/exec/Agent.java line 772:
> 
>> 770:          * Sets the maximum attempts to create or obtain an agent VM
>> 771:          * @param numAttempts number of attempts
>> 772:          * @throws IllegalArgumentException if {@code numAttempts} is lesser than {@code 1}
> 
> s/lesser/less/

Fixed.

> src/share/classes/com/sun/javatest/regtest/exec/Agent.java line 990:
> 
>> 988:         private Duration idleTimeout;
>> 989:         // default is 2 i.e. we retry a failed agent selection once
>> 990:         private int numAgentSelectionAttempts = 2;
> 
> The default should be 1, meaning that the new behavior is opt-in, not opt-out.
> 
> You can always set a different default in the layers of software wrapping `jtreg`.

I've updated the PR to use the default as 1.

> src/share/classes/com/sun/javatest/regtest/exec/Agent.java line 990:
> 
>> 988:         private Duration idleTimeout;
>> 989:         // default is 2 i.e. we retry a failed agent selection once
>> 990:         private int numAgentSelectionAttempts = 2;
> 
> The limit of 2 is set here but also later on.

I did not understand this review comment. With the updated changes in this PR, the default value now is set to 1, here too. Did you mean something more?

> src/share/classes/com/sun/javatest/regtest/tool/i18n.properties line 304:
> 
>> 302:     couldn't be obtained from the pool. Similarly, a value of 2 will imply \
>> 303:     that if the agent VM creation fails or an existing one couldn't be \
>> 304:     obtained from the pool, then 1 retry will be attempted. \
> 
> The text about "queries" seems "weird".  Suggest:
> 
> The number of attempts jtreg will make to access an agent with the desired characteristics, either by creating a new one, or obtaining one from the pool of reusable VMs. The minimum value, and the default value, is 1.

Thank you for providing this text. I agree it's much more concise and provides all the necessary details. I've updated the PR to use this text.

-------------

PR Review Comment: https://git.openjdk.org/jtreg/pull/173#discussion_r1392284422
PR Review Comment: https://git.openjdk.org/jtreg/pull/173#discussion_r1392284647
PR Review Comment: https://git.openjdk.org/jtreg/pull/173#discussion_r1392286015
PR Review Comment: https://git.openjdk.org/jtreg/pull/173#discussion_r1392287151
PR Review Comment: https://git.openjdk.org/jtreg/pull/173#discussion_r1392288077