Request for Approval: 6425769: Allow specifying an address to bind JMX remote connector

[Andrew Hughes]

----- Original Message -----
> Hi,
> 
> On Fri, 2016-02-19 at 13:38 -0500, Andrew Hughes wrote:
> > 
> > ----- Original Message -----
> > > Hi,
> > > 
> > > Please approve and sponsor the following fix for JDK-6425769 to 7u-dev.
> > 
> > I'm not sure what you mean by "sponsor". Do you mean you don't have commit
> > access and need someone else to push for you? I can do that if so.
> 
> Yes. I don't have push access. Thanks!
> 

Ok, I'll push it later today, once I've done a test build.

> > > It's a one-to-one backport from the fix which went into JDK 8 and JDK
> > > 9. What needed changing is the test to account for JDK 7 and
> > > accompanying testlibrary. There were also two subsequent test fixes in
> > > JDK 8/9 for which I'd like to ask for backport approval as well.
> > > 
> > > Main bug:
> > > 
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-6425769
> > > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6425769/jdk7.00/
> > > HG export:
> > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6425769/jdk7-exports/
> > > 
> > > The actual patch is the same as in 9 and 8. Only slight modifications
> > > to the test have been done to make it work with JDK 7.
> > 
> > This looks fine. I assume the tests ran fine on OpenJDK 7?
> 
> Yes. Test runs fine on OpenJDK 7. Fails unpatched. Passes with the
> patch. I've used:
> 
> $ JT_JAVA=/path/to/jdk jtreg \
>     test/java/lang/management \
>     test/com/sun/management \
>     test/sun/management
> 
> No change in test results (modulo the newly added test as described
> above).
> 
> > The one line I would change is:
> > 
> > (bindAddress == null ? "" : "\n\t" + PropertyNames.HOST + "=" +
> > bindAddress)
> > 
> > I don't see a need for a check as the toString result of null is "null",
> > but
> > no point changing it if it's already in 8 & 9.
> 
> This has specifically been asked for during review in 9:
> http://mail.openjdk.java.net/pipermail/jmx-dev/2015-December/000889.html
> 

Ok, I would have pointed out to them that it's not only inconsistent with the
other lines in that debug message, but that "" and null are two different possible
values.

> I'd rather keep it in sync with 8 and 9.

Yes, I agree this isn't the place to change it, as I said.

> 
> > Also, not sure why you need two URLs here? What is the difference between
> > webrev (the one I review) and 'HG export'?
> 
> Convenience. If you don't use the hg-exports, that's fine by me. Some
> people prefer that over webrev patches. I didn't mean to confuse
> people.

Ok, I'm just not sure how they are different; the exported hg patch is
in the webrev, isn't it?

> 
> > > 
> > > 
> > > Test fixes for the test introduced with JDK-6425769:
> > > 
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8145982
> > > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8145982/jdk7.00/
> > > HG export:
> > >  http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8145982/jdk7-exports/JDK-8145982.export.jdk.patch
> > > 
> > > It's the very same patch as in 8 and 9.
> > > 
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8146015
> > > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8146015/jdk7.00/
> > > HG export:
> > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8146015/jdk7-exports/JDK-8146015.export.jdk.patch
> > > 
> > > It's the same patch as in 8 and 9. It only accounts for surrounding code
> > > changes.
> > > 
> > > Please let me know if there are questions.
> > > 
> > > Thanks,
> > > Severin
> > > 
> > 
> > Looks ok to me.
> 
> Thanks for the review!
> 
> Cheers,
> Severin
> 
> 

-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222