Skip to content

PHP: add deserialze as the argument for the interceptor#15779

Merged
ZhouyihaiDing merged 1 commit intogrpc:v1.13.xfrom
ZhouyihaiDing:deserialize
Jun 21, 2018
Merged

PHP: add deserialze as the argument for the interceptor#15779
ZhouyihaiDing merged 1 commit intogrpc:v1.13.xfrom
ZhouyihaiDing:deserialize

Conversation

@ZhouyihaiDing
Copy link
Copy Markdown
Contributor

@ZhouyihaiDing ZhouyihaiDing commented Jun 15, 2018

Hi @mehrdada

For the interceptor API, I feel like adding deserialize as the argument makes more sense, because it should be couple with the method. For example, If the user changes the method through the interceptor, the response type will change and the deserialize method should change too. In another case, if the user uses something else other than protobuf, they can use the interceptor to pick whatever deserializer they want.

In addition, use($channel) in the code is enough to hide what the user doesn't needs.

Since we are releasing the PHP interceptor by 1.13.0 and now it's 1.13.0-pre, if this change make sense, I think we can backport it to 1.13.0?

@mehrdada
Copy link
Copy Markdown
Contributor

This change needs a formal gRFC.

@mehrdada
Copy link
Copy Markdown
Contributor

/cc @hsaliak

@hsaliak
Copy link
Copy Markdown
Contributor

hsaliak commented Jun 15, 2018

@ZhouyihaiDing this is an API change, so we should consider it carefully. The process to change it is necessarily heavy weight. However, since the API is not in yet, this is pretty much the last chance to change it.
So let's consider this change here before we decide if we want to re-visit the RFC.

  1. Why will the response type change cause the deserializer change? Isnt the de-serializer fixed to how the response is encoded, rather than what the data is?
  2. Do you have some use cases where we have encountered this?
    Normally, interceptors contain cross cutting concerns rather than business logic, so they only mutate data rarely.. so how often do you see users get into a problem with the current API?
  3. How do other languages do this, do they provide a deserializer?

@ZhouyihaiDing
Copy link
Copy Markdown
Contributor Author

ZhouyihaiDing commented Jun 15, 2018

Oh I already created a PR to the grpc/proposal repo. I think we can close that if the change is no needed?

It's a thing better to have, because it won't do any harm to the interceptor. To my understand, interceptor can intercept everything related to the RPC except the channel.
But it is also unnecessary, because I can't image why the user want to the deserializer.

The reason I have this idea is when I implementing the gcp extension(kind of like a plugin to the gRPC), which need to maintain it's own channel pool and do pre/post process for each RPC. This could be done by the interceptor, but since the Call and the Channel are bound too tight, I need 2 more API changes to do the similar thing. #15761 and #15749.

The purpose of this PR is decouple the RPC and Channel, kind of like CallInvoker in C# does. If we give the $deserialize back to the RPC, then the interceptor will have everything needed for creating a Call object. But this is kind of tricky because this is not the usual way to use the interceptor.

@ZhouyihaiDing
Copy link
Copy Markdown
Contributor Author

ZhouyihaiDing commented Jun 16, 2018

I finished the implementation which only uses the interceptor to hook the GCP extension support with the change of this PR: ZhouyihaiDing#22. The GCP interceptor is implemented with self maintained channel and call, which is exactly what CallInvoker implementation is doing.
I chatted with @mehrdada offline and we could merge this to 1.13.0 by state "There is an experimental API and there is a proposal under the review".

CallInvoker implemetation needs to add 2 more APIs change as described above to de-couple the channel and the call object from the stub. However, it makes the GCP implementation be hooked in a more clear way.

I am okay with adding CallInvoker and changing the interceptor, because CallInvoker will make the GCP extension looks more directly(and since the C# has it, maybe PHP can have it too), while the Inteceptor changes is doing the least changes to the grpc php code.

@ZhouyihaiDing ZhouyihaiDing changed the base branch from master to v1.13.x June 16, 2018 04:03
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do you need to move this parameter to the end and provide a default value in order not to break existing users?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Seems not that easy. The user is supposed to rewrite this function. Unless interceptUnaryUnary takes two kinds of the input: with deserialize and without deserialize.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If existing user can be broken without changing their own code, you cannot do that.
As I said, I think you should introduce this API change in a compatible way.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@TeBoring it appears that this code was never shipped as a gRPC release. That's why I asked @ZhouyihaiDing to get this on v1.13.x branch so that we would never have shipped gRPC PHP API with the existing interceptor API to alleviate existing user concerns.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see. Thanks for the explanation.

@ZhouyihaiDing
Copy link
Copy Markdown
Contributor Author

Hi, @chingor13 @michaelbausor
I am going to update the API for the interceptor to decouple the Call object and Channel object. Can you please give me some suggestions if there are some concerns relate to it? Thanks!

@ZhouyihaiDing ZhouyihaiDing changed the base branch from v1.13.x to master June 20, 2018 23:32
@ZhouyihaiDing ZhouyihaiDing changed the base branch from master to v1.13.x June 20, 2018 23:32
@ZhouyihaiDing ZhouyihaiDing changed the base branch from v1.13.x to master June 20, 2018 23:33
@ZhouyihaiDing
Copy link
Copy Markdown
Contributor Author

Sorry. I used the rebase in a wrong way and it was trying to commit tons of commits into the v1.13.x branch.

@ZhouyihaiDing ZhouyihaiDing changed the base branch from master to v1.13.x June 20, 2018 23:36
@ZhouyihaiDing ZhouyihaiDing changed the base branch from v1.13.x to master June 20, 2018 23:37
@ZhouyihaiDing
Copy link
Copy Markdown
Contributor Author

Known issues for the test:
Bazel Debug build for C/C++ - #15610
Bazel Opt build for C/C++ - #15610

@ZhouyihaiDing ZhouyihaiDing merged commit 6c7b6b8 into grpc:v1.13.x Jun 21, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Sep 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants