Vyom,
> On 12 Oct 2017, at 10:01, vyom tewari <[email protected]> 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 <[email protected]> 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 <[email protected]> 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.
>>>>>
>>>>>
>>
>
