Feat backend separate transport specific messages #343
Conversation
artek-koltun
requested review from
glimchb,
jainvipin,
GottliebNoam and
sburla-marvell
| FabricsPath fabrics = 5 [(google.api.field_behavior) = OPTIONAL]; | ||
| } | ||
|
|
||
| message FabricsPath { |
There was a problem hiding this comment.
seems like we have tuple: (addr, port, nqn) for target and again (addr, port, nqn) for source, no ?
maybe we can somehow capture this in it's own message ?
There was a problem hiding this comment.
The traddr for the target is in the common part (it's the BDF for PCIe), so it needs to be split across two objects here.
There was a problem hiding this comment.
@benlwalker WDYT about struct spdk_nvme_transport_id ?
https://github.com/spdk/spdk/blob/ee9fb405bfa6f11b75788c38752b2494598bbbd8/include/spdk/nvme.h#L455-L501
I think it is pretty good abstraction what we want to adopt here, why not ?
There was a problem hiding this comment.
I do like that abstraction. To make that work here you'd need to have a FabricsPath with a source and target transport id, and then a MemoryBusPath with just a target transport id (or just traddr), and put the two into a OneOf.
There was a problem hiding this comment.
My goal was to have the annotations as precise as possible, in order to provide a correct docs and benefit from some automatic validation tools like fieldbehavior.ValidateRequiredFields() call.
If we extract those members into a separate message, we probably need to mark nqn and port as OPTIONAL, since they are not applicable for pcie case, "lying" that they are actually required for fabrics. In the current patch, we clearly state that nqn and port are required by fabrics and not used by pcie
glimchb
added
the
Merge Candidate
| FabricsPath fabrics = 5 [(google.api.field_behavior) = OPTIONAL]; | ||
| } | ||
|
|
||
| message FabricsPath { |
There was a problem hiding this comment.
The traddr for the target is in the common part (it's the BDF for PCIe), so it needs to be split across two objects here.
artek-koltun
force-pushed
the
feat-backend-separate-transport-specific-messages
branch
from
361be99 to
4618e5f
Compare
Signed-off-by: Artsiom Koltun <artsiom.koltun@intel.com>
Signed-off-by: Artsiom Koltun <artsiom.koltun@intel.com>
Signed-off-by: Artsiom Koltun <artsiom.koltun@intel.com>
For continuation of the discussion in #330