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

Support GRPC #716

Merged
merged 26 commits into from Nov 18, 2019
Merged

Support GRPC #716

merged 26 commits into from Nov 18, 2019

Conversation

neokidd
Copy link
Contributor

@neokidd neokidd commented Jul 11, 2019

Motivation:

Support GRPC. It is based on #383. But managed to use different ways to get proxy.

Modification:

Comparing to #383, this support of GRPC uses a different approach to make proxy. It hacks byte code to allow extending GPRC interface. In this way, implementation looks more straight forward. Using this support of GRPC is also more naturally like other SOFA supported protocols.

It keeps minimal changes to core-implement. Only important change to core is a small change allowing GRPC proxy to pass interface check.

It's merged with the latest master. No conflicts. Ready for PR.

Result:

Based on #383. Fixes its conflict issues, as a different approach is used.

It adds GRPC support as an extension. Minimal touch to core.

@sofastack-bot
Copy link

sofastack-bot bot commented Jul 11, 2019

Welcome

Please sign CLA! Contributor License Agreement!

After we will automatically Sync the status of this Pull Request in Three Minutes

@sofastack-bot sofastack-bot bot added cla:no Need sign CLA First-time contributor First-time contributor size/XXL labels Jul 11, 2019
@neokidd
Copy link
Contributor Author

neokidd commented Jul 11, 2019

Looks it because of the maven plugin not downloaded. I'm fixing that.

@ujjboy ujjboy added cla:yes CLA is ok and removed cla:no Need sign CLA labels Jul 11, 2019
@neokidd
Copy link
Contributor Author

neokidd commented Jul 24, 2019

Build issue fixed. Please review.

@neokidd
Copy link
Contributor Author

neokidd commented Jul 31, 2019

@ujjboy any progress with code review?

@ujjboy ujjboy requested a review from NeGnail August 1, 2019 04:02
Copy link
Member

@ujjboy ujjboy left a comment

Choose a reason for hiding this comment

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

I will continue to review this pr when I have time.

example/pom.xml Outdated Show resolved Hide resolved
import com.alipay.sofa.rpc.core.exception.SofaRpcRuntimeException;

import com.alipay.sofa.rpc.core.request.SofaRequest;
import com.alipay.sofa.rpc.core.response.SofaResponse;
Copy link
Member

Choose a reason for hiding this comment

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

format imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will make change accordingly.

super(consumerConfig);
this.interfaceID = consumerConfig.getInterfaceId();
String[] segments = this.interfaceID.split("\\.");
String lastSegment = segments[segments.length - 1];
Copy link
Member

Choose a reason for hiding this comment

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

why not use method lastIndexOf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will make change to use method lastIndexOf

@leizhiyuan
Copy link
Contributor

@ujjboy @NeGnail @JervyShi Please review this

@leizhiyuan leizhiyuan added this to the 5.6.3 milestone Nov 6, 2019
@leizhiyuan
Copy link
Contributor

please change version to 5.6.3-SNAPSHOT

fix the build problem ,we will merge It into 5.6.3

@neokidd
Copy link
Contributor Author

neokidd commented Nov 6, 2019

Crazily busy recently, I will try to look into this over this weekend.

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@201de55). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #716   +/-   ##
=========================================
  Coverage          ?   72.03%           
  Complexity        ?     1084           
=========================================
  Files             ?      376           
  Lines             ?    15347           
  Branches          ?     2445           
=========================================
  Hits              ?    11055           
  Misses            ?     3027           
  Partials          ?     1265
Impacted Files Coverage Δ Complexity Δ
...ava/com/alipay/sofa/rpc/config/ConsumerConfig.java 75.64% <0%> (ø) 0 <0> (?)
...ava/com/alipay/sofa/rpc/config/ProviderConfig.java 54% <0%> (ø) 0 <0> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 201de55...347cdc9. Read the comment docs.

@neokidd
Copy link
Contributor Author

neokidd commented Nov 8, 2019

I will continue to review this pr when I have time.

Done with requested changes.

@neokidd
Copy link
Contributor Author

neokidd commented Nov 8, 2019

please change version to 5.6.3-SNAPSHOT

fix the build problem ,we will merge It into 5.6.3

Done.

Copy link
Contributor

@leizhiyuan leizhiyuan 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
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

@leizhiyuan leizhiyuan merged commit a87b868 into sofastack:master Nov 18, 2019
@ghost ghost mentioned this pull request Dec 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes CLA is ok First-time contributor First-time contributor size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants