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.
All usages are now ExtendedSocketOptions.getInstance().options(...).
Have you considered a static options(...) method to reduce the typing at
each of the usage sites?
-Alan
