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
> > >> >
> > >> >
> > >>
> > >>
> >
> >
> >
> >
>
>
>
>
>
