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

Triple refactor demo #2341

Closed
wants to merge 0 commits into from
Closed

Triple refactor demo #2341

wants to merge 0 commits into from

Conversation

DMwangnima
Copy link
Contributor

Core code
See /protocol/grpc_new
How to use protoc-gen-connect-go
Compile /protocol/grpc_new/protoc-gen-connect with name grpc-gen-connect-go, then move this file to GOPATH eg: /usr/local/go/bin. Then, execute this command in dubbo-go dir.
protoc --go_out=. --connect-go_out=. --go_opt=paths=source_relative --connect-go_opt=paths=source_relative ./protocol/grpc_new/connect/proto/connect/ping/v1/ping.proto
user demo
See /protocol/grpc_new/internal
Current progress and follow-up plans
Successfully adapted the Connect protocol to dubbo-go. Protocol details will be adjusted and cross-testing will be added in the future.

@@ -70,6 +70,7 @@ import (
_ "dubbo.apache.org/dubbo-go/v3/protocol/dubbo3/health"
_ "dubbo.apache.org/dubbo-go/v3/protocol/dubbo3/reflection"
_ "dubbo.apache.org/dubbo-go/v3/protocol/grpc"
_ "dubbo.apache.org/dubbo-go/v3/protocol/grpc_new"
Copy link
Member

Choose a reason for hiding this comment

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

The name should be determined.

Copy link
Contributor Author

@DMwangnima DMwangnima Jun 17, 2023

Choose a reason for hiding this comment

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

I used grpc_new to represent new protocol temporarily. The exact name will be determined later.

func NewClient(url *common.URL) (*Client, error) {
// If global trace instance was set, it means trace function enabled.
// If not, will return NoopTracer.
//tracer := opentracing.GlobalTracer()
Copy link
Member

Choose a reason for hiding this comment

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

// If global trace instance was set, it means trace function enabled.
// If not, it will return NoopTracer.
// tracer := opentracing.GlobalTracer()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to think about how to use Middleware mode to add tracer.

conRefs := config.GetConsumerConfig().References
ref, ok := conRefs[key]
if !ok {
panic("no reference")
Copy link
Member

Choose a reason for hiding this comment

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

Please print all references for debugging proposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK.

cliOpts = append(cliOpts, connect.WithGRPC())
// todo: process triple
default:
panic("not support")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
panic("not support")
panic("%s is not supported", ref.Protocol)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK.

}
}
cliOpts = append(cliOpts, connect.WithGRPC())
// todo: process triple
Copy link
Member

Choose a reason for hiding this comment

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

Will the GPRC_NEW replace the Triple protocol?

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 depends on how the composition of the triple protocol is identified. Maybe Rest(?)+GRPC ?

ref, ok := conRefs[key]
if !ok {
panic("no reference")
}
Copy link
Member

Choose a reason for hiding this comment

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

if ref, ok := conRefs[key]; !ok {
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK.

return nil, err
}
tlsFlag = true
}
Copy link
Member

Choose a reason for hiding this comment

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

if tlsConfig := config.GetRootConfig().TLSConfig; tlsConfig != nil {
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK.


key := url.GetParam(constant.InterfaceKey, "")
impl := config.GetConsumerServiceByInterfaceName(key)
conRefs := config.GetConsumerConfig().References
Copy link
Member

Choose a reason for hiding this comment

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

var (
    key = url.GetParam(constant.InterfaceKey, "")
    impl = config.GetConsumerServiceByInterfaceName(key)
    ...
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK.

default:
panic("not support")
}
cli := &http.Client{
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cli := &http.Client{
client := &http.Client{

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK.

//)
var cfg *tls.Config
tlsConfig := config.GetRootConfig().TLSConfig
if tlsConfig != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

@justxuewei
Copy link
Member

Plus, this pull request can be splited into three commits:

  1. Introduce connect-go library;
  2. Grpc_new logics;
  3. An example of the grpc_new.

@sonarcloud
Copy link

sonarcloud bot commented Jul 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

dataStr = strings.TrimPrefix(dataStr, "code_")
code, err := strconv.ParseInt(dataStr, 10 /* base */, 64 /* bitsize */)
if err == nil && (code < int64(minCode) || code > int64(maxCode)) {
*c = Code(code)

Check failure

Code scanning / CodeQL

Incorrect conversion between integer types

Incorrect conversion of a 64-bit integer from [strconv.ParseInt](1) to a lower bit size type uint32 without an upper bound check.
@sonarcloud
Copy link

sonarcloud bot commented Jul 18, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

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.

2 participants