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

Dispatch on (service, procedure, encoding) #232

Closed
HelloGrayson opened this issue Jun 24, 2016 · 6 comments
Closed

Dispatch on (service, procedure, encoding) #232

HelloGrayson opened this issue Jun 24, 2016 · 6 comments
Milestone

Comments

@HelloGrayson
Copy link
Contributor

HelloGrayson commented Jun 24, 2016

We need to decide whether or not to dispatch on the triple of (service, procedure, encoding) or not. Currently, we route on (service, procedure), and if the encoding is not correct, it will fail to serialize/deserialize.

Are there any benefits to routing on the full triple instead?

cc @yarpc/golang

@HelloGrayson HelloGrayson added this to the Finalize API milestone Jun 24, 2016
@HelloGrayson HelloGrayson changed the title Route on encoding Add encoding to dispatch tuple, making it (service, procedure, encoding) Jun 24, 2016
@HelloGrayson HelloGrayson changed the title Add encoding to dispatch tuple, making it (service, procedure, encoding) Dispatch on (service, procedure, encoding) Jun 24, 2016
@prashantv
Copy link
Contributor

In a far off future world, it'd be great if the same handler code could be used for different encodings transparently (e.g. you can make this request over Thrift or Protobuf, and the same handler logic is used to generate a response).

However, unless we have a good use case, I don't think we should expose dispatching to different handlers based on the Encoding -- that seems confusing. E.g. if a user could implement their own Meta::health for JSON that returned a completely different result that the Thrift Meta::health method.

I think disallowing registration on the triplet also leaves us room in the future to add the feature if there are good use cases.

@HelloGrayson
Copy link
Contributor Author

@abhinav @kriskowal what do you guys think?

@abhinav
Copy link
Contributor

abhinav commented Jun 24, 2016

I buy Prashant's argument. We can add this later without breaking anything, but we can't remove it once added. We'll need to validate the encodings in the handlers though. I think we have a TODO for that right now.

@HelloGrayson
Copy link
Contributor Author

We should at least determine whether or not this field should be required or not - currently it's optional.

@prashantv
Copy link
Contributor

+1 to required

@HelloGrayson
Copy link
Contributor Author

Will close this for now in favor of #238

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

3 participants