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
