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

[vyom tewari]

Hi Alan,

Thanks for review comments, please find my answers inline.

Vyom


On Monday 23 April 2018 06:04 PM, Alan Bateman wrote:
> On 23/04/2018 13:05, vyom tewari wrote:
>> Hi,
>>
>> Please find the latest 
>> webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.1/index.html). 
>> I incorporated  most of the review comments.
> This looks much better but I think the second paragraph of the spec of 
> each option needs to be inverted so that it states clearly what the 
> options does before it gets into the background of SO_KEEPALIVE. For 
> example, TCP_KEEPALIVE should say that it sets the idle period for 
> keep alive before it explains SO_KEEPALIVE. The currently wording also 
> begs questions as to whether the socket option means anything when 
> SO_KEEPALIVE is not enabled. Also it probably should say something 
> about whether it can be changed at any time or not.
>
> The implementation looks much better but the filtering by socket 
> option name is still a bit fragile. It might be better for 
> ExtendedSocketOptions to mainain 3 sets of options, one for 
> SOCK_UNSPEC, one for SOCK_STREAM, the other for SOCK_DGRAM. A map 
> would work too. The register method can specify socket type and it's 
> very obvious which sockets the option is intended to be used for.
I choose to overload the "options(short type)" over maintain multiple 
set of options because it was straight forward and  minimal changes were 
required. If i split the extended options to multiple set then it will 
increases the complexity i have to change the register method and i have 
to change "ifOptionSupported" as well. i feel it is not worth because we 
are not going to add socket options frequently.
>
> All usages are now ExtendedSocketOptions.getInstance().options(...). 
> Have you considered a static options(...) method to reduce the typing 
> at each of the usage sites?
no, let me check if we can consider static options(..) method.
>
> -Alan