Hi Serguei,
The webrev was updated to address all these comments.
Webrev: http://cr.openjdk.java.net/~dtitov/8163083/webrev.08
<http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.08>
Bug: https://bugs.openjdk.java.net/browse/JDK-8163083
Thanks!
--Daniil
*From: *<serguei.spit...@oracle.com>
*Organization: *Oracle Corporation
*Date: *Thursday, September 27, 2018 at 11:33 AM
*To: *Daniil Titov <daniil.x.ti...@oracle.com>, JC Beyler
<jcbey...@google.com>
*Cc: *<alexey.men...@oracle.com>, <gary.ad...@oracle.com>,
<serviceability-dev@openjdk.java.net>
*Subject: *Re: RFR 8163083: SocketListeningConnector does not allow
invocations with port 0
Just noticed one more minor issue:
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.LinkedHashMap;
It seems the above imports are not really used and can be removed.
Thanks,
Serguei
On 9/27/18 11:22 AM, serguei.spit...@oracle.com
<mailto:serguei.spit...@oracle.com> wrote:
Hi Daniil,
It looks great, thank you for the update.
I have a couple of more minor comments on the test.
56 testWithDefaultArgs(connector);
57 testWithDefaultArgs2(connector);
58 testWithWildcardPort1(connector);
59 testWithWildcardPort2(connector);
Please, rename testWithDefaultArgs to testWithDefaultArgs1
to make naming consistent.
111 throw new RuntimeException("Test testWithDefaultArgsNegative
failed. The argument map was not updated with" +
112 " the bound port number.");
115 // This call should fail since the previous the argument
map is already updated with the port
116 // number of the started listener
Could you, please, re-balance the above line pairs to make the
first line shorter?
No need in new webrev if you fix the above.
Thanks,
Serguei
On 9/27/18 11:00 AM, Daniil Titov wrote:
Hi Serguei,
Thank you for reviewing this change. Initially I understood
the whole fragment below (from the Javadoc for
com.sun.jdi.connect.ListeningConnector.startListening()
method) as a direction for the user how to obtain and prepare
the argument map before passing it in startListening() method.
61 * The argument map associates argument name strings to
instances
62 * of {@link Connector.Argument}. The default argument map
for a
63 * connector can be obtained through {@link
Connector#defaultArguments}.
64 * Argument map values can be changed, but map entries should
not be
65 * added or deleted.
But I agree that the last sentence could also mean that the
argument map values could be changes as a result of the method
invocation and in this case the new fragment in the Javadoc is
not needed.
Please review the updated webrev that does not add new Javadoc
fragment and includes other changes you suggested.
Webrev: http://cr.openjdk.java.net/~dtitov/8163083/webrev.07/
<http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.07/>
Bug: https://bugs.openjdk.java.net/browse/JDK-8163083
Thanks!
--Daniil
*From: *<serguei.spit...@oracle.com>
<mailto:serguei.spit...@oracle.com>
*Organization: *Oracle Corporation
*Date: *Wednesday, September 26, 2018 at 8:12 PM
*To: *Daniil Titov <daniil.x.ti...@oracle.com>
<mailto:daniil.x.ti...@oracle.com>, JC Beyler
<jcbey...@google.com> <mailto:jcbey...@google.com>
*Cc: *<alexey.men...@oracle.com>
<mailto:alexey.men...@oracle.com>, <gary.ad...@oracle.com>
<mailto:gary.ad...@oracle.com>,
<serviceability-dev@openjdk.java.net>
<mailto:serviceability-dev@openjdk.java.net>
*Subject: *Re: RFR 8163083: SocketListeningConnector does not
allow invocations with port 0
Hi Daniil,
It is great that you came up the fix for this issue.
Thanks to Gary for the help too.
I wonder if we could get away without updating the javadoc
of com/sun/jdi/connect/ListeningConnector.java.
Filing a CSR would not be needed then (simple javadoc
reformatting should not require a CSR).
So, my question is if this new fragment is really needed:
76 * <p>
77 * If the addressing information provided in {@code
arguments} implies
78 * the auto detection this information might be
updated with the address
79 * of the started listener.
The javadoc for startListening already has this fragment:
61 * The argument map associates argument name strings to
instances
62 * of {@link Connector.Argument}. The default argument map
for a
63 * connector can be obtained through {@link
Connector#defaultArguments}.
64 * Argument map values can be changed, but map entries should
not be
65 * added or deleted.
that allows to change the argument map values.
It looks like, it has to be Okay to add the bound port number there.
Please, let me know what you think.
Some formatting comments to addition to Jc's comments:
77 * If the addressing information provided in {@code
arguments} implies
78 * the auto detection this information might be
updated with the address
79 * of the started listener.
This sentence needs to be split in two:
77 * If the addressing information provided in {@code
arguments} implies
78 * the auto detection. This information might be
updated with the address
79 * of the started listener.
+
+ protected void updateArgumentMapIfRequired(
+ Map<String, ? extends Connector.Argument>
args,TransportService.ListenKey listener) {
+ }
+
The indent has to be 4, not 8.
+ if(isWildcardPort(args)) {
+ String[] address = listener.address().split(":");
+ if (address.length > 1) {
+ args.get(ARG_PORT).setValue(address[1]);
+ }
+ }
A space is missed after the first 'if'.
50 filter(c ->
51
c.name().equals("com.sun.jdi.SocketListen")).findFirst().get();
This is better to be one liner.
57 testWithDefaultArgs(connector);
58 testWithDefaultArgs2(connector);
59 testWithWildcardPort(connector);
60 testWithWildcardPort2(connector);
A suggestion is to rename above as below to have the names more
unified:
57 testWithDefaultArgs1(connector);
58 testWithDefaultArgs2(connector);
59 testWithWildcardPort1(connector);
60 testWithWildcardPort2(connector);
Thanks,
Serguei
On 9/25/18 10:32 AM, Daniil Titov wrote:
HI JC,
The webrev is updated to address this.
Webrev:
http://cr.openjdk.java.net/~dtitov/8163083/webrev.06
<http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.06>
Bug: https://bugs.openjdk.java.net/browse/JDK-8163083
Thanks!
--Daniil
*From: *JC Beyler <jcbey...@google.com>
<mailto:jcbey...@google.com>
*Date: *Monday, September 24, 2018 at 8:47 PM
*To: *<daniil.x.ti...@oracle.com>
<mailto:daniil.x.ti...@oracle.com>
*Cc: *<alexey.men...@oracle.com>
<mailto:alexey.men...@oracle.com>, <gary.ad...@oracle.com>
<mailto:gary.ad...@oracle.com>,
<serviceability-dev@openjdk.java.net>
<mailto:serviceability-dev@openjdk.java.net>
*Subject: *Re: RFR 8163083: SocketListeningConnector does
not allow invocations with port 0
Hi Daniil,
Still looks good to me :)
Nit: empty line 83 of the new test is not required!
Jc
On Mon, Sep 24, 2018 at 5:54 PM Daniil Titov
<daniil.x.ti...@oracle.com
<mailto:daniil.x.ti...@oracle.com>> wrote:
Hi Alex,
Please review the updated webrev that has new test
moved to test/jdk/comsun/jdi/connect folder.
Webrev:
http://cr.openjdk.java.net/~dtitov/8163083/webrev.05/
<http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.05/>
Bug: https://bugs.openjdk.java.net/browse/JDK-8163083
Thanks!
--Daniil
On 9/24/18, 2:56 PM, "Alex Menkov"
<alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>> wrote:
Daniil,
Just remembered that SQE requested to not add new
tests to vmTestbase
(see test/hotspot/jtreg/vmTestbase/README.md)
Could you please move the test to correct location
(I suppose it's
test/jdk/com/sun/jdi)
--alex
On 09/24/2018 14:30, Alex Menkov wrote:
> +1
>
> --alex
>
> On 09/24/2018 10:39, Gary Adams wrote:
>> Looks good to me.
>>
>> On 9/24/18, 1:25 PM, Daniil Titov wrote:
>>> Hi Alex and Gary,
>>>
>>> Please review the updated patch that includes
suggested changes.
>>>
>>>
http://cr.openjdk.java.net/~dtitov/8163083/webrev.04/
<http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.04/>
>>> Bug:
https://bugs.openjdk.java.net/browse/JDK-8163083
>>>
>>> Thanks!
>>> --Daniil
>>>
>>>
>>>
>>>
>>> On 9/21/18, 3:59 PM, "Alex
Menkov"<alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>> wrote:
>>>
>>> One more note:
>>> please add "@Override" annotation to the
>>>
SocketListeningConnector.updateArgumentMapIfRequired
>>>
>>> --alex
>>>
>>> On 09/21/2018 02:26,
gary.ad...@oracle.com <mailto:gary.ad...@oracle.com>
wrote:
>>> > Looks good to me.
>>> >
>>> > For the javadoc
>>> >
>>> > 72 *<p>
>>> > 73 * If<code>arguments</code>
contains addressing
>>> information. and
>>> > 74 * only one connection will
be accepted, the {@link
>>> #accept accept} method
>>> > 75 * can be called immediately
without calling this
>>> method.
>>> > 76 *
>>> > 77 * If the addressing information
provided
>>> in<code>arguments</code>
>>> > implies
>>> > 78 * the auto detection this
information might be updated
>>> with the address
>>> > 79 * of the started listener.
>>> >
>>> > - you could add a<p> tag if you
want to maintain the
>>> spacing in the
>>> > generated javadoc.
>>> > - I've seen other changesets
migrating to {@code ..} from
>>> the older
>>> > <code>...</code>
>>> >
>>> > It would be good to include some
negative testing.
>>> > Not sure if it is already covered in
other tests, e.g.
>>> >
>>> > args1 = defaultArguments()
>>> > startListening(args1) // bound port
updated
>>> > startListening(args1) // already
listening
>>> >
>>> >
>>> > On 9/20/18 10:59 PM, Daniil Titov wrote:
>>> >> Please review the change that fixes
the issue in
>>>
com.sun.tools.jdi.SocketListenerConnector.startListening()
method.
>>> >>
>>> >> When the argument map passed to
startListening() methods has
>>> the port number unspecified or set to zero the
port is auto detected.
>>> However, the consequent call of
startListening() methods with
>>> unspecified port number fails rather than
starts a new listener on
>>> auto detected port. This happens since the
original argument map
>>> passed to the startListening() methods is used
as a key to store the
>>> mapping to the started listeners.
>>> >>
>>> >> The fix ensures that in cases when
the port is auto detected
>>> the argument map is updated with the bound
port number.
>>> >>
>>> >> Mach5 vmTestbase_nsk_jdi tests
successfully passed.
>>> >>
>>> >>
Bug:https://bugs.openjdk.java.net/browse/JDK-8163083
>>> >>
Webrev:http://cr.openjdk.java.net/~dtitov/8163083/webrev.02/
<http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.02/>
>>> >>
>>> >> Thanks!
>>> >> --Daniil
>>> >>
>>> >>
>>> >>
>>> >
>>>
>>>
>>>
>>
--
Thanks,
Jc
