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

[question] Better GRPC package name #379

Closed
unikzforce opened this issue Jul 21, 2020 · 12 comments
Closed

[question] Better GRPC package name #379

unikzforce opened this issue Jul 21, 2020 · 12 comments
Milestone

Comments

@unikzforce
Copy link

unikzforce commented Jul 21, 2020

Could you please change the Proto package to something more specific than package api;?

Something like io.centrifugo , for example ...
Something related to your brand/organization?

In a code base with a lot of grpc dependencies, one could lose track of which package is for which organization.

@unikzforce unikzforce changed the title [question] Would you please change the GRPC package to something more specific to centrifugo organization? [question] Better GRPC package name Jul 21, 2020
@FZambia
Copy link
Member

FZambia commented Jul 21, 2020

Hi, do you mean package inside proto defintions? I suppose you can simply change it after copying? Or I am missing sth?

@unikzforce
Copy link
Author

unikzforce commented Jul 22, 2020

image
If i change line 4, I can not connect to centrifugo. Centrifugo has been compiled with that, and only would work with that package I think, unless i rebuild it myself. Currently I'm using docker.

Anyway If I could enforce that , I would rather not. I'd rather You enforce the package name to the others and that package name could be something that is hinting to your brand , Like the ones I've mentioned above( 'io.centrifugo' , 'io.centrifugal' ...)

@unikzforce
Copy link
Author

Maybe it could be changed via a Version 3 ? Possibly with a migration enforced to all users who want to switch to version 3

@FZambia
Copy link
Member

FZambia commented Jul 22, 2020

If i change line 4, I can not connect to centrifugo. Centrifugo has been compiled with that, and only would work with that package I think, unless i rebuild it myself.

Hm, this is interesting, I can't understand why changing package name in API proto definitions can lead to non-working API since I hope it's not included in network communication. Will try to check it out.

@FZambia
Copy link
Member

FZambia commented Jul 23, 2020

@unikzforce you are right, GRPC uses package in RPC method names: like: "/api.Centrifugo/Broadcast". As soon as package name changed we get an error like:

Transport level error: rpc error: code = Unimplemented desc = unknown service centrifugo.Centrifugo

This means we can't simply fix this without breaking existing generated code. Centrifugo v3 is not planned in near perspective so forcing v3 release to fix this minor thing does not look like a good plan to me.

An alternative is to create sth like v3_use_new_grpc_api_package_name boolean option and make it on by default for new projects (like v3_use_offset we already have). Though this will require having 2 identical API packages on server internally until v3 release.

@FZambia
Copy link
Member

FZambia commented Jul 23, 2020

Could you be more specific on what problem do you have with current naming? Do I understand right that setting

option java_package = "io.centrifugo";` 
option java_outer_classname = "CentrifugoProto"

options to API proto definitions solves problem for your case? Or you still have api somewhere even after adding these options?

@unikzforce
Copy link
Author

unikzforce commented Aug 2, 2020

Sorry for my delay.

I didn't have any problem, Actually Your software rocks. And worked very well for me.

you guys have created a universal package that can be used elsewhere. and it is unique. Like this:

image

for example it is the content of timestamp.proto:

image

so if you ever wanted to use Timestamp in your code you have to import it with the googles' unique google.protobuf namespace.
So If someone is creating something Unique , It's always better and more professional to have a unique namespace for the facilities she/he is introducing to the outside world. As I noticed you guys are really professional at something. So I think your protobuf packaging and namespacing should be professional too.

So suppose I have an a protobuf file , and I wanted to embed PublishRequest message inside one of my own messages (hypothetical Scenario).

It would be really more professional to import io.centrifugo.proto.PublishRequest inside my proto file insead of just importing api.PublishRequest into that file. second one will do the job, but the first one will be more clear . and will tell the reader that this PublishRequest has something to do with Centrifugo. so if he/she has a lot of proto files inside his project, his/her mind will not be crowded with a lot of possibilities while reading his own code. As he/she looks io.centrifugo.proto.PublishRequest he will remember instantly that this PublishRequest is from Centrifugo.

now every person in the world will just now that if he/she wants to use Timestamp in a protobuf file, he have to import google.protobuf.Timestamp.

Suppose google wanted to introduce a new message called PublishRequest for itself in the vague package name api.
as you also did that. then conflicts will come.

It's obvious that google would never choose io.centrifugo.proto as namespace.
And It's Obvious that you guys would never choose google.protobuf is a namespace for your messages.

If any individual entity or organization choose a unique namespace for itself, and use that as the package name for it's messages, then the conflicts would be less likely.

@unikzforce
Copy link
Author

My argument was not about the name of java packages, but it was about the name of protobuf file packages.

@FZambia
Copy link
Member

FZambia commented Aug 3, 2020

@unikzforce thanks for description, the motivation is more clear to me now. Will consider changing this in Centrifugo v3 when it comes. Though at moment I can't say when v3 release happen.

@FZambia FZambia added this to the v3.0.0 milestone Apr 22, 2021
@FZambia
Copy link
Member

FZambia commented Jun 11, 2021

Going to rename it to centrifugal.centrifugo.api in v3 (without io part - for example official Google proto files do not use domain zone as part of package name - example)

@KabDeveloper
Copy link

Going to rename it to centrifugal.centrifugo.api in v3 (without io part - for example official Google proto files do not use domain zone as part of package name - example)

Changing the package name will prevent others to fall on the error I had too.

@FZambia
Copy link
Member

FZambia commented Sep 2, 2021

Done in v3 🎉

@FZambia FZambia closed this as completed Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants