adding rsockets support into JDK
Lu, Yingqi
yingqi.lu at intel.com
Thu Nov 15 17:22:46 UTC 2018
Hi Chris,
Thank you for your detailed review. Yes, only RdmaSockets and RdmaSocketOptions have JDK specific specs.
I will send version 17 this morning.
Thanks,
Lucy
>-----Original Message-----
>From: Chris Hegarty [mailto:chris.hegarty at oracle.com]
>Sent: Thursday, November 15, 2018 9:12 AM
>To: Lu, Yingqi <yingqi.lu at intel.com>
>Cc: nio-dev at openjdk.java.net; Viswanathan, Sandhya
><sandhya.viswanathan at intel.com>; Aundhe, Shirish
><shirish.aundhe at intel.com>; Kaczmarek, Eric <eric.kaczmarek at intel.com>
>Subject: Re: adding rsockets support into JDK
>
>Lucy,
>
>> http://cr.openjdk.java.net/~ylu/8195160.16/
>
>In terms of review, I’m going to focus on the specification, since that is the
>immediate priority to get the CSR finalized. Given the last few emails, I think
>that the implementation is in fairly reasonable shape anyhow.
>
>Only two files have spec impact, and I will comment on them separately
>( please correct me if these two are not the only files containing JDK-specific
>spec) :
>
>1) src/jdk.net/share/classes/jdk/net/RdmaSocketOptions.java
>
> a) Class-level description contains:
> "Defines socket options specific to rsocket."
>
> Suggest:
> "Defines socket options specific to RDMA-based TCP sockets and
> channels."
>
> b) All options start with "Set the integer size ..." - it should just
> be "The integer size ..."
>
>2) src/jdk.net/share/classes/jdk/net/RdmaSockets.java
>
> a) As already agreed, please remove the following superfluous methods:
> `setOption` x2, `getOption` x2, `supportedOptions`. And the
> verbiage in the class-level description that relates to these.
>
> b) I'm ok wherever these factory methods end up, either in `Sockets`
> or in `RdmaSockets`. If they stay in `RdmaSockets` then `Rdma` can
> be removed from their name, e.g. `RdmaSockets::openSocket`.
>
> c) Is it possible to just remove references to `rsocket`? there are
> currently 4., e.g. "@apiNote The rsocket implementation on Linux
> only supports IPv4 addresses." Surely there is likely to be just
> one implementation. I wonder also if this should be an *impl*Note
> rather than an *api*Note.
>
> d) If keeping the `RdmaSockets` class, then please remove the many
> `@since 12` javadoc tags from each method, and just add a single
> class-level tag.
>
>-Chris.
>
>
>> On 14 Nov 2018, at 17:06, Lu, Yingqi <yingqi.lu at intel.com> wrote:
>>
>> Hi Chris,
>>
>> Thank you for your feedback.
>>
>> I was following the suggestion [1] from Joe Darcy to move the RDMA factory
>methods from jdk.net.Sockets to jdk.net.RDMASockets.
>>
>> I will remove setOption and getOption methods from jdk.net.RdmaSockets
>in the next version of the patch. At the meantime, please let me know if there
>is anything else missing from the current version.
>>
>> Thanks,
>> Lucy
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8205186
>>
>>> -----Original Message-----
More information about the nio-dev
mailing list