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

Refactor subspace-networking module structure. #1867

Merged
merged 6 commits into from
Aug 24, 2023

Conversation

shamil-gadelshin
Copy link
Member

This PR changes the module structure of the subspace-networking crate.

Changes:

  • rename create module to composer as well as the related function. We should use a noun for the module name and neither "creation" nor "creator" seem appropriate.
  • move all custom protocols to the protocols module
  • move all request handlers and request-reponses request factory to requests module
  • rename request_handlers module to handlers (requests::handlers)
  • introduce mod.rs for protocol and requests modules instead of extra files in the related folders

Code contributor checklist:

- gather custom protocols and request handlers in the “protocol” module
@shamil-gadelshin shamil-gadelshin added the networking Subspace networking (DSN) label Aug 23, 2023
@shamil-gadelshin shamil-gadelshin self-assigned this Aug 23, 2023
Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

I don't think I like compose name. Maybe we can make it a constructor on Node? We have precedents with constructors returning a tuple of instance being constructed + worker, in this case NodeRunner is a worker.

Also please don't use mod.rs. We don't have them in the repo and we don't need to introduce them, better use module_name.rs as we did prior to this.

@nazar-pc
Copy link
Member

nazar-pc commented Aug 23, 2023

Also requests is a bit confusing since it is both requests and responses, maybe request_response or whatever it is called in libp2p itself?

@shamil-gadelshin
Copy link
Member Author

I don't think I like compose name. Maybe we can make it a constructor on Node? We have precedents with constructors returning a tuple of instance being constructed + worker, in this case NodeRunner is a worker.

I don't want to overload node.rs, also this "constructor" has enough logic to be a separate entity. WDUT about build and builder similar to SDK naming?

Also please don't use mod.rs. We don't have them in the repo and we don't need to introduce them, better use module_name.rs as we did prior to this.

I wanted to reduce the file number in the root folder, and protocols.rs seemed redundant. No problem though.

Also requests is a bit confusing since it is both requests and responses, maybe request_response or whatever it is called in libp2p itself?

We have a separate request-response module responsible for the request protocols factory. I imagined requests to combine the request-response and all the handlers.

@nazar-pc
Copy link
Member

I don't want to overload node.rs, also this "constructor" has enough logic to be a separate entity. WDUT about build and builder similar to SDK naming?

If we have a builder data structure with useful methods - sure, otherwise I see nothing wrong with having it as a method in Node. It is not great when files are large, but better than moving things somewhere just for the sake of doing it.

We have a separate request-response module responsible for the request protocols factory. I imagined requests to combine the request-response and all the handlers.

Should those handler be in a submodule of request-response then?

@shamil-gadelshin
Copy link
Member Author

If we have a builder data structure with useful methods - sure, otherwise I see nothing wrong with having it as a method in Node. It is not great when files are large, but better than moving things somewhere just for the sake of doing it.

It's correct if we follow the GoF pattern definition. So, I renamed compose to construct and left the original entity composition untouched. Your original entity division (node, node_runner, creator) was proven convenient because all the entities grew complex separately and if they were coupled they'd needed to be refactored to separated entities similar to what they are today.

Should those handler be in a submodule of request-response then?

I would leave those separate because request-responses is independent and this extra-dependency won't bring any benefit at all. However, I renamed request to request-response as you suggested and renamed request-responses to request-responses-factory. We confused request-responses with the libp2p protocol with a similar name for some time. This renaming should help to avoid confusion from now on.

Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

I would leave those separate because request-responses is independent and this extra-dependency won't bring any benefit at all. However, I renamed request to request-response as you suggested and renamed request-responses to request-responses-factory. We confused request-responses with the libp2p protocol with a similar name for some time. This renaming should help to avoid confusion from now on.

I do not see why they are separate. One is inherently used with another. And I it made sense to call it what it was despite libp2p having the same module name. It just doesn't look consistent right now. While factory makes sense for what it does, from libp2p point of view Behaviour was the consistent name, I don't see why we want to have that specific protocol follow a different naming convention all of a sudden.

crates/subspace-networking/src/behavior.rs Outdated Show resolved Hide resolved
-  rename RequestResponseFactory to RequestResponseFactoryBehaviour
@shamil-gadelshin
Copy link
Member Author

I do not see why they are separate. One is inherently used with another. And I it made sense to call it what it was despite libp2p having the same module name. It just doesn't look consistent right now. While factory makes sense for what it does, from libp2p point of view Behaviour was the consistent name, I don't see why we want to have that specific protocol follow a different naming convention all of a sudden.

I'm not sure I understood you 100%. The request factory is an abstract entity and doesn't know about its usage. Different handlers could be passed to it even from the outer crates and having handlers to be submodule of factory is not necessary. However, the "-Behaviour" suffix is probably a good idea, I initially omitted it because of the long name but since it immediately caused confusion it's better to add it back.

Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

I'm not sure I understood you 100%. The request factory is an abstract entity and doesn't know about its usage. Different handlers could be passed to it even from the outer crates and having handlers to be submodule of factory is not necessary. However, the "-Behaviour" suffix is probably a good idea, I initially omitted it because of the long name but since it immediately caused confusion it's better to add it back.

What I meant is that it would be a factory if it produced multiple things, but it is a single behavior, not multiple, hence factory as part of the name doesn't make a lot of sense to me, it is simply not a factory. RequestResponseFactoryBehaviour is a bit less bad name in a sense that it is one behavior, but it kind of instantiates multiple request response protocols internally.

Please squash this PR on merge.

@shamil-gadelshin shamil-gadelshin enabled auto-merge (squash) August 24, 2023 06:44
@shamil-gadelshin shamil-gadelshin merged commit a65d460 into main Aug 24, 2023
9 checks passed
@shamil-gadelshin shamil-gadelshin deleted the refactor-networking-module-structure branch August 24, 2023 08:18
vedhavyas pushed a commit that referenced this pull request Aug 25, 2023
Refactor the module structure of the `subspace-networking` crate: gather custom protocols and request handlers in the “protocols” module.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
networking Subspace networking (DSN)
Projects
Development

Successfully merging this pull request may close these issues.

2 participants