adding rsockets support into JDK

Lu, Yingqi yingqi.lu at intel.com
Thu Nov 15 19:12:29 UTC 2018


Hi Chris,

Here is the version 17 of the patch: http://cr.openjdk.java.net/~ylu/8195160.17/

In this version, I changed the two files from the implementation side: RdmaSockets.java and RdmaSocketOptions.java according to your suggestions. In addition, since I change the function names from openRdmaXXX to openXXX inside RdmaSockets.java, I also updated all the jtreg tests.

Please double check and let me know if there is anything missing from the two files.

Thanks,
Lucy

>-----Original Message-----
>From: nio-dev [mailto:nio-dev-bounces at openjdk.java.net] On Behalf Of Lu,
>Yingqi
>Sent: Thursday, November 15, 2018 9:23 AM
>To: Chris Hegarty <chris.hegarty at oracle.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
>
>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