Hi Chris,

Thanks for review. Please find the latest webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.5/index.html).

Thanks,

Vyom


On Saturday 14 October 2017 02:25 AM, Chris Hegarty wrote:
Vyom,

On 12 Oct 2017, at 10:01, vyom tewari <vyom.tew...@oracle.com> 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).
This looks much better.

Some suggested wordings of the JDK-specific option:

/**
  * Disable Delayed Acknowledgements.
  *
  * <p> This socket option can be used to reduce or disable delayed
  * acknowledgments (ACKs).
  *
  * <p> The value of this socket option is a {@code Boolean} that represents
  * whether the option is enabled or disabled. The socket option is specific
  * to stream-oriented sockets using the TCP/IP protocol.
  *
  * <p> The exact semantics of this socket option are socket type and system
  * dependent.
  *
  * <p> When TCP_QUICKACK is enabled, ACKs are sent immediately, rather than
  * delayed if needed in accordance to normal TCP operation. This option is
  * not permanent, it only enables a switch to or from TCP_QUICKACK mode.
  *
  * <p> Subsequent operations of the TCP protocol will once again enter/leave
  * TCP_QUICKACK mode depending on internal protocol processing and factors
  * such as delayed ack timeouts occurring and data transfer.
  *
  * @since 18.3
  */

-Chris.

P.S. D’oh, sorry, of course you need the paragraph tags.


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 <vyom.tew...@oracle.com> 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 <vyom.tew...@oracle.com> 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 <p>, 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.



Reply via email to