RFR 8145635 : Add TCP_QUICKACK socket option

[Langer, Christoph]

Hi Vyom,

looks good to me.

Best regards
Christoph

> -----Original Message-----
> From: net-dev [mailto:net-dev-bounces at openjdk.java.net] On Behalf Of
> vyom tewari
> Sent: Donnerstag, 19. Oktober 2017 04:56
> To: net-dev at openjdk.java.net
> Subject: Re: RFR 8145635 : Add TCP_QUICKACK socket option
> 
> Hi All,
> 
> please find the latest
> webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.7/index.htm
> l).
> 
> Thanks,
> 
> Vyom
> 
> 
> On Wednesday 18 October 2017 08:21 PM, Langer, Christoph wrote:
> > Hi Vyom,
> >
> > I just looked at your latest webrev which in general looks fine to me.  Some
> minor remarks:
> >
> > make/lib/Lib-jdk.net.gmk:
> > - copyright year needs to be updated
> >
> > src/java.base/unix/classes/java/net/PlainDatagramSocketImpl.java:
> > - private void addExtSocketOptions(...) looks a bit awful concerning its
> formatting. I suggest something like this:
> >
> > private void addExtSocketOptions(Set<SocketOption<?>> extOptions,
> >                                   Set<SocketOption<?>> options) {
> >      // TCP_QUICKACK is applicable for TCP Sockets only.
> >      extOptions.stream()
> >          .filter((option) -> !option.name().equals("TCP_QUICKACK"))
> >          .forEach((option) -> options.add(option));
> > }
> >
> > src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java:
> > - copyright year needs to be updated
> > - the equals sign should be placed on line 98 here:
> >
> > 98     public static final SocketOption<Boolean> TCP_QUICKACK
> > 99             = new ExtSocketOption<Boolean>("TCP_QUICKACK",
> Boolean.class);
> >
> > Other than that you should consider it reviewed from my end. No need for
> further webrev...
> >
> > Best regards
> > Christoph
> >
> >> -----Original Message-----
> >> From: net-dev [mailto:net-dev-bounces at openjdk.java.net] On Behalf Of
> >> vyom tewari
> >> Sent: Dienstag, 17. Oktober 2017 10:37
> >> To: net-dev at openjdk.java.net
> >> Subject: Re: RFR 8145635 : Add TCP_QUICKACK socket option
> >>
> >> Hi Roger,
> >>
> >> Thanks for the review , please find the updated
> >>
> webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.6/index.htm
> >> l).
> >>
> >> Thanks,
> >>
> >> Vyom
> >>
> >>
> >> On Tuesday 17 October 2017 06:35 AM, Roger Riggs wrote:
> >>> Hi Vyom,
> >>>
> >>> A few suggestions:
> >>>
> >>> PlainDatagramSocketImpl.java:
> >>>   - line 95/96:  I think you can use just forEach, the order version is
> >>> not necessary.
> >>>      The code will be a bit more readable if the .filter and .forEach
> >>> are on a new line and don't wrap.
> >>>      You can also remove the extra "(" and ")
> >>>
> >>>   - line 87-94: these are confusing and imply some implicit resetting
> >>> of the option.
> >>>   - use @since 10
> >>> - 209/268: the native setQuickAck method should use boolean as its
> >>> argument to enable/disable
> >>>    Since enable is a boolean; it does not need "== true'
> >>>
> >>> LinuxSocketOptions.java/c:
> >>>    - 52: setQuickAck0 should use boolean for the 2nd argument; (The
> >>> native code already does)
> >>>
> >>> Thanks, Roger
> >>>
> >>>
> >>> On 10/15/17 11:58 PM, vyom tewari wrote:
> >>>> Hi Chris,
> >>>>
> >>>> Thanks for review. Please find the latest
> >>>>
> >>
> webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.5/index.htm
> >> l).
> >>>> Thanks,
> >>>>
> >>>> Vyom
> >>>>