I have asked Hamlin to hold off on this for a day or so. I have an alternative proposal that eliminates the free port anti-pattern.
-Chris. > On 29 Sep 2016, at 14:55, Roger Riggs <roger.ri...@oracle.com> wrote: > > Hi Hamlin, > > One more suggested improvement. Instead of two copy/paste copies of the > launch with options code, > it would cleaner to create a separate RMID.launch(String[] options) method > that would be passed the extra arguments. > Use it in forceLogSnapshot.java and ShutdownGracefully.java. > > The (current) no-arg version could call the version with args with an empty > array(or null). > There would be only 1 copy of the launch algorithm. > > Roger > > > > On 9/27/2016 11:22 PM, Hamlin Li wrote: >> Hi Roger, >> >> Thank you for reviewing. >> Please check the new webrev: >> http://cr.openjdk.java.net/~mli/8085192/webrev.01/, and comments inline >> below. >> >> On 2016/9/27 23:14, Roger Riggs wrote: >>> Hi Hamlin, >>> >>> Marking each test that uses RMID.launch with the bugid does not seem to be >>> meaningful >>> since the bug is in the support infrastructure of the test and not specific >>> to the test itself. >>> It would be overkill to try to confirm the bug was fixed by running all >>> those tests. >>> Putting the bugid on 1 of the tests would be sufficient. >> Remove all bug ids except of the one in CheckImplClassLoader.java. >>> >>> JavaVM.java: >>> >>> - 134-138: Why define these private methods if they are not going to be >>> used in outputContains? >>> >>> - 185: Its inefficient to convert the byte array to a string for when >>> looking for each string. >>> It would be cleaner for JavaVM to have public outputString and >>> errorString methods >>> and check them separately in RMID. >>> (remove the JavaVM.outputContains method which hides which stream the >>> string appeared in). >> Fixed. >>> (It would be a bigger change to change this to use the test library >>> ProcessTools and OutputAnalyzer). >> Thank you suggestion. Yes, you're right, it will be a little bit complicated >> to use ProcessTools and OutputAnalyzer in this situation, need to modify >> these classes, because they will block until process terminates. So I prefer >> to use current implementation, as it's simple and clear. >> >> Thank you >> -Hamlin >>> >>> Roger >>> >>> >>> On 9/27/2016 5:22 AM, Hamlin Li wrote: >>>> Please review the fix for JDK-8085192. The fix checks whether it fails to >>>> launch rmid due to "Port already in use" error, it will launch rmid again >>>> and again(20 times at most) until no such issue. >>>> >>>> bug: >>>> https://bugs.openjdk.java.net/browse/JDK-8085192 >>>> webrev: >>>> http://cr.openjdk.java.net/~mli/8085192/webrev.00/ >>>> >>>> Thank you >>>> -Hamlin >>> >> >
