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

[vyom tewari]

On Saturday 12 May 2018 04:14 PM, Ivan Gerasimov wrote:
> 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.
>
thanks for detail explanation :) , i did not saw the code but i was 
under impression that it will use size as the nearest prime number for 
uniform distribution of elements in general.

> 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 :)
if no further comment from others as well, i will change it to use 
default constructor at time of pushing.

Thanks,
Vyom

>
> 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.
>>>>>
>>>>
>>>>
>>>
>>
>>
>