RFR 8145635 : Add TCP_QUICKACK socket option

Langer, Christoph christoph.langer at sap.com
Wed Oct 18 14:51:48 UTC 2017


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
> >>
> >



More information about the net-dev mailing list