Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat backend separate transport specific messages #343

Conversation

artek-koltun
Copy link
Contributor

For continuation of the discussion in #330

@artek-koltun artek-koltun marked this pull request as ready for review August 30, 2023 06:59
@artek-koltun artek-koltun requested a review from a team as a code owner August 30, 2023 06:59
@artek-koltun artek-koltun added the Storage APIs or code related to storage area label Aug 30, 2023
@artek-koltun artek-koltun requested a review from a team August 30, 2023 07:00
FabricsPath fabrics = 5 [(google.api.field_behavior) = OPTIONAL];
}

message FabricsPath {
Copy link
Member

@glimchb glimchb Sep 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 glimchb added the Merge Candidate in the open merge window, next candidate for merge label Sep 12, 2023
FabricsPath fabrics = 5 [(google.api.field_behavior) = OPTIONAL];
}

message FabricsPath {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@glimchb glimchb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@artek-koltun artek-koltun force-pushed the feat-backend-separate-transport-specific-messages branch from 361be99 to 4618e5f Compare September 19, 2023 10:10
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge Candidate in the open merge window, next candidate for merge Storage APIs or code related to storage area
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants