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

[Protocol] 建议 .proto 中的修改 rpc register 为 Register或者其它 #662

Closed
songzhian opened this issue Dec 12, 2017 · 6 comments
Closed
Assignees
Labels
bug Something isn't working and you are sure it's a bug!
Milestone

Comments

@songzhian
Copy link

register 在 c/c++中是关键子, 生成的方法名 编译会报错 没办法友好的支持c/c++程序
建议 .proto 中的修改 register 为 Register或者其它
.proto 代码如下
service ApplicationRegisterService {
rpc register (Application) returns (ApplicationMapping) {
}
}

@wu-sheng wu-sheng added this to the 5.0-2018-preview milestone Dec 12, 2017
@wu-sheng wu-sheng added the bug Something isn't working and you are sure it's a bug! label Dec 12, 2017
@wu-sheng wu-sheng changed the title 建议 .proto 中的修改 rpc register 为 Register或者其它 [Protocol] 建议 .proto 中的修改 rpc register 为 Register或者其它 Dec 12, 2017
@wu-sheng
Copy link
Member

@OpenSkywalking/pmc and @hanahmily

This is the a request from c/c++ platform. register is definitely a key word in c++. I suggest to change the method name.

@songzhian Is ApplicationRegisterService#register the only service which has this problem?

@wu-sheng
Copy link
Member

Renaming

  • ApplicationRegisterService#register - > ApplicationRegisterService#batchRegister
  • Application -> Applications
  • Application#applicationCode -> Applications#applicationCodes
  • ApplicationMapping -> ApplicationMappings
  • ApplicationMapping#application -> ApplicationMappings#applications

New service definition:

//register service for ApplicationCode, this service is called when service starts.
service ApplicationRegisterService {
    rpc batchRegister (Applications) returns (ApplicationMappings) {
    }
}

message Applications {
    repeated string applicationCodes = 1;
}

message ApplicationMappings {
    repeated KeyWithIntegerValue applications = 1;
}

@peng-yongsheng
Copy link
Member

+1

@wu-sheng wu-sheng removed the Voting label Dec 14, 2017
@wu-sheng
Copy link
Member

With collector side support, I will do the adjustments as soon as the ASF repo is ready. Sadly, it is still not...

@wu-sheng
Copy link
Member

This will be fixed by #690 , and released in 5.0.0-alpha.

@songzhian
Copy link
Author

非常感谢 采纳我提 "c/c++ 的 register关键字问题
但是 应用实例发现服务 proto/DiscoveryService.proto#L11-L12 中的
service InstanceDiscoveryService {
rpc register (ApplicationInstance) returns (ApplicationInstanceMapping) { }
}
还是有 register 关键字
以下是 github的链接
https://github.com/apache/incubator-skywalking/blob/master/apm-network/src/main/proto/DiscoveryService.proto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working and you are sure it's a bug!
Projects
None yet
Development

No branches or pull requests

5 participants