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

Use protobuf only for RPC calls #18

Merged
merged 4 commits into from
Jun 4, 2018
Merged

Use protobuf only for RPC calls #18

merged 4 commits into from
Jun 4, 2018

Conversation

felipejfc
Copy link
Contributor

This will make it possible to have servers made in other programming languages to send and receive RPC calls to go servers.

  • Remotes should now receive and return structs that implements proto.Message interface
  • session.Data and context are now json encoded instead of gob for sending in RPC calls

This will make it possible to create cluster and RPC libs for other
programming languages
@coveralls
Copy link

coveralls commented Jun 1, 2018

Pull Request Test Coverage Report for Build 191

  • 65 of 113 (57.52%) changed or added relevant lines in 11 files are covered.
  • 16 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.6%) to 77.654%

Changes Missing Coverage Covered Lines Changed/Added Lines %
service/util.go 11 13 84.62%
component/method.go 11 14 78.57%
rpc.go 2 6 33.33%
modules/unique_session.go 0 8 0.0%
service/remote.go 16 29 55.17%
metrics/prometheus_reporter.go 0 18 0.0%
Files with Coverage Reduction New Missed Lines %
modules/unique_session.go 1 0.0%
component/method.go 1 80.2%
session/session.go 2 96.13%
rpc.go 4 66.67%
service/remote.go 8 59.6%
Totals Coverage Status
Change from base Build 185: -0.6%
Covered Lines: 3270
Relevant Lines: 4211

💛 - Coveralls

Copy link
Contributor

@cscatolini cscatolini left a comment

Choose a reason for hiding this comment

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

👍 nice job
I like the idea of removing gob but as far as I understood these changes will also remove the ability of making RPC calls with more than one argument. Are we ok with this tradeoff?

@@ -91,7 +91,9 @@ func (a *Remote) Kick(ctx context.Context) error {
if a.Session.UID() == "" {
return constants.ErrNoUIDBind
}
b, err := util.GobEncode([]byte(a.Session.UID()))
b, err := proto.Marshal(&protos.KickMsg{
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we just byte encode the string like this: []byte(a.Session.UID())? or are you using a proto since you may wan't to send more data along with the ID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RPC must receive structs that implements proto.Message, a byte array doesn't

e2e/e2e_test.go Outdated
@@ -57,10 +57,10 @@ func TestHandlerCallToFront(t *testing.T) {
data []byte
resp []byte
}{
{"connector.testsvc.testrequestonlysessionreturnsptr", []byte(``), []byte(`{"code":200,"msg":"hello"}`)},
{"connector.testsvc.testrequestonlysessionreturnsptr", []byte(``), []byte(`{"Code":200,"Msg":"hello"}`)},
Copy link
Contributor

Choose a reason for hiding this comment

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

we should keep our json keys as camel case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is because of protobuf, more details below

@@ -50,8 +50,7 @@ func configureBackend() {

func configureFrontend(port int) {
configureJaeger("connector")
ws := acceptor.NewWSAcceptor(fmt.Sprintf(":%d", port))
tcp := acceptor.NewTCPAcceptor(fmt.Sprintf(":%d", port+1))
tcp := acceptor.NewTCPAcceptor(fmt.Sprintf(":%d", port))
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you remove ws support? we're no longer accepting it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we didn't, it's just that we are not using the ws acceptor for nothing in this example

@@ -52,3 +52,16 @@ message Error {
string msg = 2;
map<string, string> metadata = 3;
}

message KickMsg {
string UserID = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

userId

}

message KickAnswer {
bool Kicked = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

kicked

}

message BindMsg {
string UID = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

uid, fid

"github.com/topfreegames/pitaya/constants"
"github.com/topfreegames/pitaya/route"
)

// RPC calls a method in a different server
func RPC(ctx context.Context, routeStr string, reply interface{}, args ...interface{}) error {
return doSendRPC(ctx, "", routeStr, reply, args...)
func RPC(ctx context.Context, routeStr string, reply proto.Message, arg proto.Message) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

we no longer accept RPC calls with several arguments?

@felipejfc felipejfc merged commit fdc512d into master Jun 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants