RFR 8145635 : Add TCP_QUICKACK socket option

[vyom tewari]

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 <vyom.tewari at 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.tewari at 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.
>>>>
>>>>
>