Hi Hamlin,
Reexport.java:
I think this change to use a separate process for the 2nd registry
changes the test so that it does not
address the original test case. The original problem was the incorrect
retention of an object in
the object table when the create of a registry in the same process failed.
Finally being able to create the registry in the same process assured
that the object was not
retained in the object table.
Go back to creating the 2nd registry in the test process.
RegistryVM.java: 2, the copyright update should be "2017, 2018," range.
(I'm really not a fan of all the RegistryVM methods with the same name
and minimal and implicit
differences in their functions. When reading the test, you have to go
and read the RegistryVM method
to see what it does. I would have preferred that the full
createRegistryVM (out, err, options, port) method
was used directly in the tests. In the case of a method used once, it
is an inconvenience method, not a convenience).
The new terminate() method is quite similar to the existing cleanup()
method which does not wait.
It would be a good cleanup to figure out if another method is really needed.
The override is going to change the behavior of the existing uses of
terminate().
It should be checked that it does not break any existing uses.
178: 187: The method comments are not consistent, one says forcibly and
the other gracefully
but both call requestExit and both call destroy().
Thanks, Roger
On 4/3/2018 11:35 PM, Hamlin Li wrote:
Hi Joe, Roger,
Thank you for reviewing, I have refactored the test more and fix as
you suggested.
please review http://cr.openjdk.java.net/~mli/8188897/webrev.01/
Thank you
-Hamlin
On 04/04/2018 10:42 AM, Joseph D. Darcy wrote:
Hello,
Some general comments on the coding for tests like this:
* It is preferable to avoid sleep in tests to avoid increasing the
minimum amount of time a test takes to run. This helps limit the
overall time it takes the test suite to run.
* If timeouts are used, it is recommend to factor the maximum time
waited with the jtreg timeout scaling factor; I don't recall its
exact name. In other words, many of our tests are run on heavily
loaded systems and the tests take longer than run than on typical
laptops and workstations so jtreg is invoked with an timeout scaling
factor. Individual tests should be sensitive to this scaling factor
for any internal timeout that need to be used.
HTH,
-Joe
On 4/3/2018 7:48 AM, Roger Riggs wrote:
Hi Hamlin,
Instead of a simple time delay, it would be useful to wait for the
RegistryVM to terminate.
In killRegistry: 149, adding subreg.waitFor() should be sufficient.
58: If using a 'for' loop it would be easier to understand if it
included the usual start, increment and termination.
Instead of burying it in the exception handler.
59, 102, 104: the introduction of the kill boolean makes the test
harder to understand and seems to be unnecessary.
the killRegistry() method already will only kill the subprocess if
it still is alive.
Roger
On 4/2/2018 6:33 AM, Hamlin Li wrote:
would you please review the following patch?
bug: https://bugs.openjdk.java.net/browse/JDK-8188897
webrev: http://cr.openjdk.java.net/~mli/8188897/webrev.00/
Thank you
-Hamlin
