Thank you, Chris, Severin, and Alex, for reviewing this change!
Best regards,
Daniil
On 7/18/19, 10:44 AM, "Alex Menkov" <alexey.men...@oracle.com> wrote:
+1
--alex
On 07/18/2019 08:27, Chris Plummer wrote:
> Hi Daniil,
>
> Looks good.
>
> Chris
>
> On 7/17/19 6:26 PM, Daniil Titov wrote:
>> Hi Chris,
>>
>> Yes, I added output.reportDiagnosticSummary() in webrev-01, but
>> removed it
>> In webrev-02, and later restored it in webrev-03.
>>
>> When the test runs of retries output.getExitValue() is set to 1
>> (COMMUNICATION_ERROR_EXIT_VAL)
>> (the "exitValue =1 " in the previous email).
>>
>> Please review a new version of the fix that has an explicit error
>> message printed
>> if the test runs out of retries (line 168).
>>
>> Webrev: http://cr.openjdk.java.net/~dtitov/8221303/webrev.04/
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8221303
>>
>> Thanks!
>> --Daniil
>>
>> On 7/17/19, 5:33 PM, "Chris Plummer" <chris.plum...@oracle.com> wrote:
>>
>> Hi Daniil,
>> I'm confused now. I mentioned output.reportDiagnosticSummary()
>> because I
>> thought I saw it in your webrev-02. Now I don't. Maybe I had glanced
>> back at webrev-01 and saw it there. One issue with exceptions in the
>> output that are not considered errors is that if later there is an
>> error, mdash wills show the exception as one of (possible multiple)
>> reasons for the failure, so it's good to avoid if possible. Looks
>> like
>> what you have now is ok.
>> I have a question about what happens if you run out of retries.
>> What is
>> output.getExitValue() set to? Also, I think you should print an an
>> explicit error message indicating you've run out of retries.
>> thanks,
>> Chris
>> On 7/17/19 4:36 PM, Daniil Titov wrote:
>> > Hi Chris,
>> >
>> > output.reportDiagnosticSummary() prints the output from the
>> process (both stdout and stderr) and the exit value to the test's stderr.
>> > In case if the port is already in use it prints the following:
>> >
>> > stdout: [];
>> > stderr: [Error: JMX connector server communication error:
>> service:jmx:rmi://127.0.0.1:9101
>> > jdk.internal.agent.AgentConfigurationError:
>> java.rmi.server.ExportException: Port already in use: 9101; nested
>> exception is:
>> > java.net.BindException: Address already in use
>> > at
>>
jdk.management.agent/sun.management.jmxremote.ConnectorBootstrap.exportMBeanServer(ConnectorBootstrap.java:820)
>>
>> > at
>>
jdk.management.agent/sun.management.jmxremote.ConnectorBootstrap.startRemoteConnectorServer(ConnectorBootstrap.java:479)
>>
>> > at
>> jdk.management.agent/jdk.internal.agent.Agent.startAgent(Agent.java:447)
>> > at
>> jdk.management.agent/jdk.internal.agent.Agent.startAgent(Agent.java:599)
>> > Caused by: java.rmi.server.ExportException: Port already in
>> use: 9101; nested exception is:
>> > < I skipped the rest>
>> > ]
>> > exitValue = 1
>> >
>> > It makes sense to have it called if the test fails, otherwise
>> this information would be missed in the test output.
>> > Please review the new version of the fix that has the call to
>> output.reportDiagnosticSummary() restored.
>> >
>> > Thanks!
>> >
>> > Webrev: http://cr.openjdk.java.net/~dtitov/8221303/webrev.03/
>> > Bug: https://bugs.openjdk.java.net/browse/JDK-8221303
>> >
>> > -Daniil
>> >
>> > On 7/17/19, 3:58 PM, "Chris Plummer"
>> <chris.plum...@oracle.com> wrote:
>> >
>> > What does output.reportDiagnosticSummary() print out then
>> the port is
>> > already in use, and have you made this happen with your
>> fixes in place?
>> >
>> > Chris
>> >
>> > On 7/17/19 3:20 PM, Daniil Titov wrote:
>> > > Hi Chris and Alex,
>> > >
>> > > Please review a new version of the fix that moves the
>> diagnostic output for the test failure to run()
>> > > method after the number of retry attempts is exceeded.
>> It also includes other corrections that
>> > > you suggested.
>> > >
>> > > Thanks!
>> > >
>> > > Webrev:
>> http://cr.openjdk.java.net/~dtitov/8221303/webrev.02/
>> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8221303
>> > >
>> > > --Best regards,
>> > > Daniil
>> > >
>> > >
>> > > On 7/17/19, 1:47 PM, "Chris Plummer"
>> <chris.plum...@oracle.com> wrote:
>> > >
>> > > Hi Daniil,
>> > >
>> > > I think you can remove "Ok" from the following
>> message:
>> > >
>> > > 237 System.out.println("DEBUG: OK. Spawned java
>> process terminated
>> > > with expected exit code of "
>> > > 238 +
>> STOP_PROCESS_EXIT_VAL);
>> > >
>> > > It's somewhat misleading since the test can still
>> fail. I think the
>> > > following is also misleading:
>> > >
>> > > 249 if (testFailed) {
>> > > 250
>> output.reportDiagnosticSummary();
>> > > 251 if
>> (output.getStderr().contains("Port already in
>> > > use")) {
>> > > 252 // Need to retry
>> > > 253 return true;
>> > > 254 }
>> > > 255 }
>> > >
>> > > The test can still pass after this happens, right?
>> And I think there are
>> > > other "Test FAILURE" error message that can appear
>> just before this.
>> > > Perhaps the code above should add a println that
>> indicates that there
>> > > will be a retry attempt since the failure was due
>> to the port being in use.
>> > >
>> > > Otherwise looks good.
>> > >
>> > > thanks,
>> > >
>> > > Chris
>> > >
>> > > On 7/17/19 1:30 PM, Alex Menkov wrote:
>> > > > Hi Daniil,
>> > > >
>> > > > The fix looks good in general.
>> > > > Couple cosmetic notes (no new webrev required):
>> > > >
>> > > > 162 needRetry = runTest();
>> > > > 163 }
>> > > > 164 while (needRetry && (attempts++
>> < MAX_RETRY_ATTEMTS));
>> > > > Please move "while" to the prev line:
>> > > > 163 } while (needRetry &&
>> (attempts++ < MAX_RETRY_ATTEMTS));
>> > > >
>> > > >
>> > > > 242 System.out.println("Test
>> FAILURE on" + name +
>> > > > " reason: The expected line \"" + READY_MSG
>> > > > 243 + "\" is not
>> present in the process
>> > > > output");
>> > > > Please add space: "Test FAILURE on " + name
>> > > >
>> > > > --alex
>> > > >
>> > > > On 07/17/2019 12:46, Daniil Titov wrote:
>> > > >> Hi Chris,
>> > > >>
>> > > >>> It's a little unclear to me why you moved
>> from ProcessThread to
>> > > >>> TestProcessThread + Process. An explanation
>> of that would make it
>> > > >>> easier
>> > > >>> to understand many of the changes.
>> > > >>
>> > > >> There are two reasons for that:
>> > > >> 1) For every network interface the test starts
>> a separate thread
>> > > >> that runs the test for this interface. We want
>> that if the test fails
>> > > >> due to the bind error the thread not to
>> exit but try to repeat
>> > > >> the test several times.
>> jdk.test.lib.thread.ProcessThread doesn't
>> > > >> allow this.
>> > > >> 2) To filter out the cases when the test fails
>> due to the bind error
>> > > >> we need to parse the process output.
>> jdk.test.lib.thread.ProcessThread
>> > > >> registers its own pumps for stdin and sdtout
>> (by calling
>> > > >> ProcessTools.startProcess()) that consume all
>> output of the process
>> > > >> and prevent
>> > > >> the output analyzer from collecting this output.
>> > > >> Thanks!
>> > > >> --Daniil
>> > > >>
>> > > >> On 7/17/19, 12:11 PM, "Chris Plummer"
>> <chris.plum...@oracle.com> wrote:
>> > > >>
>> > > >> Hi Daniil,
>> > > >> It's a little unclear to me why you
>> moved from
>> > > >> ProcessThread to
>> > > >> TestProcessThread + Process. An explanation
>> of that would make
>> > > >> it easier
>> > > >> to understand many of the changes.
>> > > >> thanks,
>> > > >> Chris
>> > > >> On 7/11/19 10:16 AM, Daniil Titov wrote:
>> > > >> > Please review the change that fixes an
>> intermittent failure of
>> > > >>
>> sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java
>> > > >> > test due to ports collision. The tests
>> finds all network
>> > > >> interfaces and for every interface starts a
>> separate process that tests
>> > > >> > the connection to JMX agent server for a
>> specific
>> > > >> ports/interface combination.
>> > > >> >
>> > > >> > The test was changed to retry in case of
>> the failure. If the
>> > > >> subprocess fails to bind and the number
>> > > >> > of retry attempts doesn't exceed the
>> limit a new pair of
>> > > >> random ports is selected and the test is run again.
>> > > >> >
>> > > >> > Webrev:
>> http://cr.openjdk.java.net/~dtitov/8221303/webrev.01/
>> > > >> > Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8221303
>> > > >> >
>> > > >> > Thanks!
>> > > >> > --Daniil
>> > > >> >
>> > > >> >
>> > > >>
>> > > >>
>> > >
>> > >
>> > >
>> > >
>> >
>> >
>> >
>> >
>> >
>>
>>
>
>
