Hi Vyom! On Fri, 2020-06-26 at 15:55 +0530, Vyom Tiwari wrote: > Hi Severin, > > Overall code changes looks ok to me, I build & tested at my local REL > it worked fine for me.
Thanks for the review! > Below are few minor comments. > > 1-> Net.java > 1.1-> I think you don't need to explicitly convert value to integer and > then pass it. You can avoid the local int variable creation as follows. > ExtendedOptionsImpl.setTcpKeepAliveIntvl(fd, (Integer)value); > 1.2-> In getSocketOption you don't need to explicitly cast it to Integer. > return ExtendedOptionsImpl.getTcpKeepAliveIntvl(fd); > 2-> PlainSocketImpl.java > 1.1 -> In setOption(SocketOption<T> name, T value) you can avoid the > local int variable. Should all be fixed. > 3-> Any specific reason for two set of functions "setTcpKeepAliveTime0 and > setTcpKeepAliveTime" for all three socket options ? Not really, other than keeping the backport aligned to the JDK 11u patch. I've updated it in the latest webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8194298/jdk8/05/webrev/ Thanks, Severin > Thanks, > Vyom > > On Fri, Jun 26, 2020 at 1:08 PM Severin Gehwolf <sgehw...@redhat.com> wrote: > > Hi, > > > > On Thu, 2020-06-25 at 23:55 +0000, Bernd Eckenfels wrote: > > > This would be a great addition. > > > > Thanks. > > > > > I do not understand why it does not support the options available for > > > Windows. Especially given the fact that it actually implements 6 > > > native methods to print "Unsupported". > > > > > > But I guess that's less a question to the backport and more to the > > > general implementation. > > > > Yes, indeed. This should be brought up for discussion in JDK head first > > and implemented there before we can consider a backport. > > > > Thanks, > > Severin > > > > > >
