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

yarpcDispatcherFn and yarpcStarterFn in modules/rpc should be public #245

Closed
yichengq opened this issue Feb 13, 2017 · 4 comments
Closed

Comments

@yichengq
Copy link
Contributor

as long as they are used in public functions, they should be public IMO.

@ascandella
Copy link
Contributor

Do you have a use case for the type? I realize it's a bit awkward, but this only exists so we have a hook to register uber-datacenter-internal dispatcher logic, which our users should never know about / care about / see

@yichengq
Copy link
Contributor Author

I don't have one, but I still feel that they should be public. If we have use cases inside our datacenter, this sounds sufficient proof that it could be used by other developers too. The function has already been public, and hiding input parameter type cannot avoid people using it.

For "our users should never know about / care about / see", I totally agree and this gonna be a general pattern. I could foresee that all modules would need uber-specific logic at least for metrics and tracing. If we continue with this approach, it means that all module packages would have a global controller and a public function to register it. I think this may overuse global structs, and building customized dispatcher while ModuleCreateFunc is more native.

I don't have a good solution though. Maybe we need an internal version which includes uber-specific logic as default. :(

@ghost
Copy link

ghost commented Feb 13, 2017

I think Aiden meant why we need Register functions :)

As per function types, it doesn't really matter:
https://golang.org/ref/spec#Type_identity

Two function types are identical if they have the same number of parameters and result values, corresponding parameter and result types are identical, and either both functions are variadic or neither is. Parameter and result names are not required to match.

So yeah, for consistency we should make them public

@ghost
Copy link

ghost commented Feb 13, 2017

#247

@ghost ghost closed this as completed Feb 13, 2017
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants