refactor(storage): combine frontend and backend messages #380
Conversation
| int64 trsvcid = 1 [(google.api.field_behavior) = REQUIRED]; | ||
|
|
||
| // Destination fabrics endpoint | ||
| FabricsEndpoint dst = 1 [(google.api.field_behavior) = REQUIRED]; | ||
|
|
||
| // Subsystem NQN | ||
| string subnqn = 2 [(google.api.field_behavior) = REQUIRED]; |
There was a problem hiding this comment.
Want to discuss a possibility to move subnqn from NvmePath into NvmeRemoteController. I see NvmeRemoteController as a view into a remote subsystem, which can be reached by means of multiple NvmePaths. However, 2 NvmePaths from a single NvmeRemoteController cannot lead to different subsystems, so, in my opinion, it would make sense to put subnqn into the controller to state a subsystem we want to reach by means of multiple paths.
The same about hostnqn. We can state our hostnqn when we define our view to the subsystem.
If we need subnqn and hostnqn for a path, we can get that information from a corresponding controller,w e can find by means of controller_name_ref
@benlwalker could you please comment?
| // see https://github.com/aip-dev/google.aip.dev/issues/1147 for field_behavior annotations | ||
| oneof path { | ||
| // Required for pcie transport type. Shall contain BDF | ||
| string pcie = 4 [(google.api.field_behavior) = OPTIONAL]; |
There was a problem hiding this comment.
Should we try to use PciEndpoint here for symmetry with frontend?
There was a problem hiding this comment.
Done. Added comments how PcieEndpoint values correspond to BDF and added domain_id. Pls take a look
artek-koltun
changed the title
|
|
||
| // Required for Nvme over fabrics transport types | ||
| FabricsPath fabrics = 5 [(google.api.field_behavior) = OPTIONAL]; | ||
| } | ||
| } | ||
|
|
||
| // Represents Fabrics transport path parameters | ||
| message FabricsPath { |
There was a problem hiding this comment.
I was thinking we get rid of FabricsPath and use only FabricsEndpoint it is too confusing to have them both...
There was a problem hiding this comment.
Removed. Now the field behavior annotations are a bit less restrictive, but it may be ok for us
There was a problem hiding this comment.
source_* members are moved to FabricsEndpoint, but would it make sense to move it to NvmeRemoteControlelr, since they are only backend related parameters?
|
merged #382 |
Signed-off-by: Artsiom Koltun <[email protected]>
Signed-off-by: Artsiom Koltun <[email protected]>
Signed-off-by: Artsiom Koltun <[email protected]>
Signed-off-by: Artsiom Koltun <[email protected]>
Signed-off-by: Artsiom Koltun <[email protected]>
artek-koltun
force-pushed
the
refactor-combine-frontend-and-backend-messages
branch
from
23d319f to
60cc80e
Compare
Signed-off-by: Artsiom Koltun <[email protected]>
resolves #376