PHP: add deserialze as the argument for the interceptor#15779
PHP: add deserialze as the argument for the interceptor#15779ZhouyihaiDing merged 1 commit intogrpc:v1.13.xfrom
Conversation
|
This change needs a formal gRFC. |
|
/cc @hsaliak |
|
@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.
|
|
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. 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 |
|
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 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 |
e1ef879 to
15d3e62
Compare
src/php/lib/Grpc/Interceptor.php
Outdated
There was a problem hiding this comment.
Do you need to move this parameter to the end and provide a default value in order not to break existing users?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
|
Hi, @chingor13 @michaelbausor |
15d3e62 to
0785cc0
Compare
|
Sorry. I used the rebase in a wrong way and it was trying to commit tons of commits into the v1.13.x branch. |
0785cc0 to
d6b483b
Compare
Hi @mehrdada
For the interceptor API, I feel like adding
deserializeas the argument makes more sense, because it should be couple with themethod. For example, If the user changes themethodthrough 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?