Re: RFR 8145635 : Add TCP_QUICKACK socket option

[vyom tewari]

Hi All,

please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.7/index.html).

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-boun...@openjdk.java.net] On Behalf Of
vyom tewari
Sent: Dienstag, 17. Oktober 2017 10:37
To: net-dev@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