Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

triple invoke support callback&future #1249

Merged
merged 15 commits into from
Nov 9, 2022

Conversation

zhenjunMa
Copy link
Contributor

背景

目前SOFA-RPC的triple协议不支持callback调用,该RP预期增加该能力

实现思路

整体上希望基于gRPC本身的Observer机制实现,如下:

  1. 用户在使用callback方式时,仍旧按照现有的编程界面,设置callbackhandler且仍然按照同步方法发起调用

  2. 当用户设置callback时,请求会到TripleClientTransport中的asyncSend方法,这里实现方案仿造syncSend方法,把请求交给TripleInvoker的asyncInvoke方法

  3. 在TripleInvoker的asyncInvoke中做了方法切换,比如用户调用的是test(request)方法,这里会转换成调test(request, StreamObserver)方法,然后让StreamObserver的实现是调用用户设置的callbackhandler

其他

请首先评估一下上述实现思路是否合理,如果合适的话再补充测试用例

@sofastack-bot sofastack-bot bot added cla:yes CLA is ok size/M labels Sep 23, 2022
@zhenjunMa
Copy link
Contributor Author

@OrezzerO @EvenLjj 有时间帮忙review下哈🙏

@codecov
Copy link

codecov bot commented Sep 25, 2022

Codecov Report

Merging #1249 (08be3c8) into master (d53562a) will decrease coverage by 0.26%.
The diff coverage is 49.51%.

@@             Coverage Diff              @@
##             master    #1249      +/-   ##
============================================
- Coverage     72.21%   71.95%   -0.27%     
+ Complexity      782      780       -2     
============================================
  Files           412      413       +1     
  Lines         17467    17555      +88     
  Branches       2723     2739      +16     
============================================
+ Hits          12614    12631      +17     
- Misses         3466     3531      +65     
- Partials       1387     1393       +6     
Impacted Files Coverage Δ
...sofa/rpc/transport/triple/TripleClientInvoker.java 61.65% <44.00%> (-26.02%) ⬇️
.../sofa/rpc/message/triple/TripleResponseFuture.java 50.00% <50.00%> (ø)
...fa/rpc/transport/triple/TripleClientTransport.java 71.66% <72.22%> (+0.79%) ⬆️
...java/com/alipay/sofa/rpc/module/LookoutModule.java 52.38% <0.00%> (-33.34%) ⬇️
...ipay/sofa/rpc/server/bolt/BoltServerProcessor.java 64.86% <0.00%> (-4.73%) ⬇️
...n/java/com/alipay/sofa/rpc/common/SofaConfigs.java 84.90% <0.00%> (-1.89%) ⬇️
.../alipay/sofa/rpc/metrics/lookout/RpcLookoutId.java 87.30% <0.00%> (-1.59%) ⬇️
...om/alipay/sofa/rpc/server/triple/TripleServer.java 74.86% <0.00%> (-1.07%) ⬇️
.../com/alipay/sofa/rpc/context/RpcInvokeContext.java 81.48% <0.00%> (-0.93%) ⬇️
...ipay/sofa/rpc/tracer/sofatracer/RpcSofaTracer.java 90.32% <0.00%> (-0.75%) ⬇️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sofastack-bot sofastack-bot bot added size/L and removed size/M labels Sep 27, 2022
@zhenjunMa zhenjunMa requested a review from OrezzerO September 27, 2022 09:40
@zhenjunMa zhenjunMa changed the title triple invoke support callback triple invoke support callback&future Sep 27, 2022
@EvenLjj EvenLjj added this to the 5.9.0 milestone Sep 28, 2022
Copy link
Collaborator

@EvenLjj EvenLjj left a comment

Choose a reason for hiding this comment

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

LGTM

@EvenLjj EvenLjj requested review from chuailiwu and JervyShi November 8, 2022 06:09
Copy link
Member

@JervyShi JervyShi left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@chuailiwu chuailiwu left a comment

Choose a reason for hiding this comment

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

LGTM

@EvenLjj EvenLjj merged commit 2a0556d into sofastack:master Nov 9, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants