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

consistent naming #14

Closed
sskaur opened this issue Apr 5, 2022 · 5 comments
Closed

consistent naming #14

sskaur opened this issue Apr 5, 2022 · 5 comments

Comments

@sskaur
Copy link
Contributor

sskaur commented Apr 5, 2022

The gateway service RPCs such as bdev_rbd_create, nvmf_delete_subsystem, and nvmf_subsystem_remove_host were originally implemented as 1:1 in naming to SPDK methods. As a result, these names use both create and add as well as delete and remove. These words have different connotations, but we may want to rename these methods to be more consistent.

  1. Should the RPCs use a consistent naming scheme such as action_prefix + component or should they mirror SPDK methods?
  2. Should the RPCs universally use add/remove, add/delete, create/remove, or create/delete?

Additionally, the cli commands do not necessarily need to match the target RPCs, but we may want consistency in the naming of these as well. Currently, the cli commands follow the scheme action_prefix + component with the prefixes create_ delete_ and get_, and there's been discussion in #9 about the need for add_ in the case of adding hosts to a subsystem.

  1. Should the cli commands universally use add/remove, add/delete, create/remove, or create/delete?

The purpose of this issue is to discuss the best plan for consistent naming for both cli commands and RPCs.

@PepperJo
Copy link

PepperJo commented Apr 6, 2022

IMO I think it is fine if we use both create and add since they have different semantics. For delete and remove I'm not sure if there is such a different, so I would fix one. My personal preference would be remove.
In regards to the CLI I think whatever we use for the RPCs we should mirror to the cli commands for consistency.

@sskaur
Copy link
Contributor Author

sskaur commented Apr 6, 2022

In that case, I think it makes sense to use create_bdev, create_subsystem, and create_transport since these are somewhat independent components. That leaves add_namespace, add_host, and add_listener since these components are created and then added to another component.

IMO, delete is semantically closely tied to create whereas remove means to take something away from something else so it is more closely tied to add.

I agree with mirroring the RPCs for the CLI commands.

@trociny
Copy link
Contributor

trociny commented Apr 6, 2022

That leaves add_namespace, add_host, and add_listener since these components are created and then added to another component.

Can the same namespace be added to different subsystems? If yes, then "add" looks better than "create". Otherwise I would prefer "create" (although I don't have a strong opinion). The same is for the listener.

@PepperJo
Copy link

PepperJo commented Apr 7, 2022

That leaves add_namespace, add_host, and add_listener since these components are created and then added to another component.

I guess these are semantically add_namespace to subsystem and add_host to subsystem. For the listener I'm not sure if we should split it into create_listener and add_listener since you should be able to use the same ip/port for multiple subsystems.

@sskaur
Copy link
Contributor Author

sskaur commented Sep 19, 2022

As decided in #31, the RPCs use a consistent naming scheme which is mirrored in CLI commands. The scheme follows action_prefix + component with action_prefix to be either create/delete for independent components or add/remove for dependent components.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

3 participants