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

[Ivan Gerasimov]

Thanks Vyom!

I like it much better now.

The last minorish comment:

src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java
Set<SocketOption<?>> options = new HashSet<>(5);

Please note that the constructor of HashSet wants the initial capacity 
as the argument, not the expected number of elements.
So in this case it would be more accurate to have (7), so that 7 * 0.75 
= 5.25 > 5.
Practically, it wouldn't make any difference, as both 5 and 7 would be 
rounded up to 8 anyways.

However, I would recommend using the default constructor just to avoid 
confusion.
Or, alternatively, collect the options to an ArrayList of desired 
capacity and then make unmodifiable set with Set.copyOf(list).

No further comments from my side :)

With kind regards,
Ivan


On 5/12/18 2:21 AM, vyom tewari wrote:
> 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.
>>>>
>>>
>>>
>>
>
>

-- 
With kind regards,
Ivan Gerasimov