Thanks, Dan for the review. I pushed it [1] and added a 9-bp label to the bug. I'll try to downport this once jdk9u-dev is open.
Best regards Christoph [1] http://hg.openjdk.java.net/jdk10/jdk10/jdk/rev/04ad8f0efc06 From: Daniel D. Daugherty [mailto:[email protected]] Sent: Samstag, 15. Juli 2017 00:27 To: Langer, Christoph <[email protected]> Cc: [email protected]; Andrew Leonard <[email protected]> Subject: Re: RFR 8183123 : JDP packets have no processId context set On 7/13/17 5:34 AM, Langer, Christoph wrote: Hi Daniel, here is the updated webrev: http://cr.openjdk.java.net/~clanger/webrevs/8183123.1/<http://cr.openjdk.java.net/%7Eclanger/webrevs/8183123.1/> src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java No comments. test/sun/management/jdp/JdpOnTestCase.java No comments. Thumbs up. Dan To me it looks ok, I'll update copyright header when I submit. Thumbs up from you? Thanks Christoph From: Andrew Leonard [mailto:[email protected]] Sent: Freitag, 7. Juli 2017 15:20 To: [email protected]<mailto:[email protected]> Cc: [email protected]<mailto:[email protected]>; Langer, Christoph <[email protected]><mailto:[email protected]> Subject: Re: RFR 8183123 : JDP packets have no processId context set Hi Daniel, Thank you for the review. You actually make a good observation, I had intended to just change that method implementation to use the ProcessHandle class, however the pid() method returns a "long" and the containing method returns an Integer, so it needs "lossy" casting to an (int) before "boxing" to an Integer. Thinking again about this, given this is just a private method within this class that is only called from one place, it seems cleaner to change the private method to return a Long object, and change the calling instance appropriately. I also see if I look at the javadoc for ProcessHandle.pid() that it can in "theory" return UnsupportOperationException, so I have also handled that. I have a new webrev, which I will ask Christoph to upload... Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: [email protected]<mailto:[email protected]> From: "Daniel D. Daugherty" <[email protected]<mailto:[email protected]>> To: Andrew Leonard <[email protected]<mailto:[email protected]>>, [email protected]<mailto:[email protected]> Date: 05/07/2017 21:27 Subject: Re: RFR 8183123 : JDP packets have no processId context set ________________________________ On 6/29/17 7:57 AM, Andrew Leonard wrote: Hi All, Please can I get some review feedback for my changes for this issue: https://bugs.openjdk.java.net/browse/JDK-8183123 The webrev patch has been uploaded here: http://cr.openjdk.java.net/~clanger/webrevs/8183123.0/<http://cr.openjdk.java.net/%7Eclanger/webrevs/8183123.0/> src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java L137 return (int)ProcessHandle.current().pid(); The return type is Integer. Why not cast to "Integer" instead of "int"? test/sun/management/jdp/JdpOnTestCase.java The test update just verifies that a non-NULL PROCESS_ID is found, but doesn't verify the format (integer) of the return. Of course, a platform independent format for PROCESS_ID might be problematic... For example, in some versions of Cygwin, I've seen negative values for PIDs... Thumbs up. If you change the cast I don't need to see a new webrev. Dan Essentially the fix entails: - Replacing invalid process id query logic with call to ProcessHandle.current().getPid(). - Update testcase to cover the failing scenario. Thus it fails without my patch, and succeeds with it. Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: [email protected]<mailto:[email protected]> Unless stated otherwise above: IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU Unless stated otherwise above: IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
