RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

Hi Ivan,

Thanks for review, please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.4/index.html). 
Please find my answers in-line.

Thanks,

vyom


On Saturday 12 May 2018 11:15 AM, Ivan Gerasimov wrote:
> Hi Vyom!
>
> 1)
> src/java.base/share/classes/sun/net/ext/ExtendedSocketOptions.java
>
> Thank you for fixing ExtendingSocketOption.options0().
> It may be better to make the returned set unmodifiable, and then 
> Collectors.toUnmodifiableSet could be used for convenience:
>
> return options.stream()
>         .filter((option) -> !option.name().startsWith("TCP_"))
>         .collect(Collectors.toUnmodifiableSet());
>
> Also, I think Alan suggested to filter out UDP_* options for 
> SOCK_STREAM here.
fixed
>
> 2)
> src/java.base/unix/classes/java/net/PlainDatagramSocketImpl.java
>
> If you static-import ExtendedSocketOptions.SOCK_DGRAM as in other 
> files, then the line 80 wouldn't become too long.
>
> The same applies to 
> src/java.base/unix/classes/java/net/PlainSocketImpl.java
fixed
>
> 3)
> src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java
>
> Was it intentional that when flowSupported == true, quickAckSupported 
> == false, keepAliveOptSupported == true, the TCP_KEEP* options are 
> *not* added to the resulting set?
>
> If not, then can this set population be organized in more linear way:  
> Just create an ArrayList, conditionally fill it in and return 
> unmodifiable set with Set.of(list.toArray()).
of course not, i  don't know but i always prefer complex nested 
"if-else" then linear ifs :)  fixed as well.
>
> Nit:  please place the equal sign at line 172 consistently with the 
> other two inits above.
fixed.
>
> With kind regards,
> Ivan
>
>
> On 5/11/18 7:43 PM, vyom tewari wrote:
>> thanks all for review, please find the latest 
>> webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.3/index.html). 
>> I address most of the  review comments.
>>
>> Vyom
>>
>>
>> On Saturday 12 May 2018 12:01 AM, Chris Hegarty wrote:
>>> On 11 May 2018, at 01:04, Alan Bateman <Alan.Bateman at oracle.com> wrote:
>>>> On 10/05/2018 16:21, vyom tewari wrote:
>>>>> Hi,
>>>>>
>>>>> Please find the latest 
>>>>> webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.2/index.html) 
>>>>>
>>>>> ...
>>>> It would be better if the channel implementation didn't static 
>>>> import ExtendedSocketOptions.getInstance as that is a very generic 
>>>> method method name. As I mentioned previously, you could simplify 
>>>> all these usages if you add the following to 
>>>> sun.net.ext.ExtendedSocketOption
>>>>     static Set<SocketOption<?>> options(int type) { return 
>>>> getInstance().options(type)); }
>>> +1
>>>
>>>> A minor comment on tests is that they can use List.of instead of 
>>>> Arrays.asList.
>>> +1
>>>
>>> Otherwise, this looks very good.
>>>
>>> -Chris.
>>>
>>> P.S. A separate issue, but when reviewing this it reminded me that 
>>> we should deprecate-for-removal jdk/net/Sockets.java. It’s 
>>> functionality is already supported by a standard API.
>>>
>>
>>
>