Hi Alan,
thanks for pointing out, i am forwarding it to net-dev list.
Vyom
On Thursday 12 October 2017 03:54 PM, Alan Bateman wrote:
Best to reply on net-dev as that is where the main review should be
going on (seems there are at two review threads going, maybe they
could unite on net-dev).
On 12/10/2017 10:01, vyom tewari wrote:
Hi Roger,
Thanks for the review, i incorporated all review comments from you
except("you can use ExtendedSocketOptions.TCP_QUICKACK to check for
the option to omit without
embedding the name."). ExtendedSocketOptions.java is part of
"jdk.net" so we can not directly use it in java.base, hence i used
the option name to filter "TCP_QUICKACK".
Please find the update
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.4/index.html).
Thanks,
Vyom
On Wednesday 11 October 2017 08:46 PM, Roger Riggs wrote:
Hi Vyom,
Comments:
- update copyright
- use @since 18.3 instead of @since 10
- ExtendedSocketOptions.java: 265,269 include the "TCP_QUICKACK" in
the exception messages.
Line 144: if you are going to keep the assert, add a explanation
about how it could happen.
I'd remove the assert.
- The first sentence should be a complete sentence: "TCP_QUICKACK
socket option enables sending TCP/IP acks immediately" or similar.
- A reference to the appropriate TCP/IP spec for quickack would be
a good addition. At least the RFC # and section.
- 85: space after "." The phrasing is a bit odd in places in the
javadoc.
- line 81/82 the value is true to enable and false to disable.
- This phrase is confusing: "it only enables a switch to or from
TCP_QUICKACK mode";
Since it is set on a socket, it should remain set on that socket
until it is changed.
- 203: be consistent in using enable/disable in parameters, etc
even for private methods.
"on" -> "enable"
PlainDatagramSocketImpl: 89:
Why create a new set with zero or one items just to throw it away?
Use the iterator to add only the non-TCP_QUICKACK options to the
supported options.
Also, you can use ExtendedSocketOptions.TCP_QUICKACK to check for
the option to omit without
embedding the name.
Sockets.java:
- The initialization of isQuickAckAvailable might be cleaner as an
nested static class
that initializes the value in its static initializer. That would
delay the init til needed
and avoid extra flags.
LinuxSocketOptions.java:
- the native methods should be static; since the instance is unused.
- line 41: the return type should be Boolean
- line 46: the return type of getQuickAck0 should be Boolean like
the argument to set.
- line 34: using JNU_ThrowByNameWithLastError directly is
sufficient; if the errno does not have a string unix supplies
"unknown error nnn".
Regards, Roger
On 10/10/2017 2:58 PM, Chris Hegarty wrote:
Vyom,
What about suggestion 1) below, the name of the socket option?
-Chris.
On 27 Sep 2017, at 09:56, vyom tewari wrote:
Hi Chris,
Thanks for review, please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.2/index.html).
I incorporated review comments from you and re-base the patch to
our consolidated repo(jdk10/master).
Thanks,
Vyom
On Monday 25 September 2017 01:57 AM, Chris Hegarty wrote:
Vyom,
On 11 Sep 2017, at 16:38, vyom tewari
wrote:
Hi All,
As jdk.net API already moved out of java.base, Please review the
below code change for jdk10.
Bug: https://bugs.openjdk.java.net/browse/JDK-8145635
Webrev:
http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html
This looks quite good. Some comments:
1) I wonder if we should just call the option TCP_QUICKACK,
rather than SO_QUICKACK? Similar to TCP_NODELAY.
2) I think LinuxSocketOptions.h is largely unnecessary, if we do
1) above.
3) Java_jdk_net_LinuxSocketOptions_getQuickAck could return jint,
rather than jobject, thus avoiding the need for createBoolean.
The conversation can happen in the Java layer. Can you please
reduce the lint length in this file, by wrapping similar to the
style of the Solaris version.
4) ExtendedSocketOptions.java
- drop the , they are unnecessary.
- I think, similar to TCP_NODELAY, the spec should say that
the options is TCP specific. From TCP_NODELAY: "The socket option
is specific to stream-oriented sockets using the TCP/IP protocol.”
- "In TCP_QUICKACK mode”, maybe “When the option is enabled…”
-Chris.
