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

Ftr: Triple Protocol Support #1071

Merged
merged 45 commits into from
Apr 3, 2021
Merged

Conversation

LaurenceLiZhixin
Copy link
Contributor

What this PR does:

Add Triple Protocol Support

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


@codecov-io
Copy link

codecov-io commented Mar 2, 2021

Codecov Report

Merging #1071 (0c5f3f2) into 3.0 (a347a04) will decrease coverage by 0.71%.
The diff coverage is 63.57%.

❗ Current head 0c5f3f2 differs from pull request most recent head 2d54f5c. Consider uploading reports for the commit 2d54f5c to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##              3.0    #1071      +/-   ##
==========================================
- Coverage   60.03%   59.32%   -0.72%     
==========================================
  Files         260      272      +12     
  Lines       12855    13350     +495     
==========================================
+ Hits         7718     7920     +202     
- Misses       4174     4453     +279     
- Partials      963      977      +14     
Impacted Files Coverage Δ
cluster/cluster_impl/available_cluster.go 100.00% <ø> (ø)
cluster/cluster_impl/broadcast_cluster_invoker.go 76.47% <ø> (ø)
cluster/cluster_impl/failfast_cluster_invoker.go 66.66% <ø> (ø)
cluster/cluster_impl/failsafe_cluster_invoker.go 80.00% <ø> (ø)
cluster/cluster_impl/zone_aware_cluster_invoker.go 64.28% <ø> (ø)
cluster/directory/static_directory.go 60.60% <ø> (ø)
cluster/loadbalance/consistent_hash.go 87.75% <ø> (ø)
cluster/loadbalance/least_active.go 90.24% <ø> (ø)
cluster/loadbalance/random.go 100.00% <ø> (ø)
cluster/loadbalance/util.go 65.00% <ø> (ø)
... and 219 more

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 9119d58...2d54f5c. Read the comment docs.

@AlexStocks AlexStocks added this to the v3.0.0 milestone Mar 7, 2021
go.mod Outdated
)

replace (
github.com/coreos/bbolt => go.etcd.io/bbolt v1.3.4
github.com/envoyproxy/go-control-plane => github.com/envoyproxy/go-control-plane v0.8.0
github.com/shirou/gopsutil => github.com/shirou/gopsutil v0.0.0-20181107111621-48177ef5f880
go.etcd.io/bbolt v1.3.4 => github.com/coreos/bbolt v1.3.3
Copy link
Contributor

Choose a reason for hiding this comment

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

go.etcd.io/bbolt v1.3.4 => github.com/coreos/bbolt v1.3.4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

protocol.BaseInvoker
// the net layer client, it is focus on network communication.
client *dubbo3.TripleClient
quitOnce sync.Once
Copy link
Contributor

Choose a reason for hiding this comment

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

quitOnce for which object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, for quit dubbo3 invoker.

dubbo3 "github.com/dubbogo/triple/pkg/triple"
)

// DubboInvoker is implement of protocol.Invoker. A dubboInvoker refer to one service and ip.
Copy link
Member

Choose a reason for hiding this comment

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

Please refact implementation follow

#1045

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

"github.com/apache/dubbo-go/common/logger"
"github.com/apache/dubbo-go/config"
"github.com/apache/dubbo-go/protocol"
dubbo3 "github.com/dubbogo/triple/pkg/triple"
Copy link
Member

Choose a reason for hiding this comment

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

move it up to the 2rd import block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed !

@@ -18,6 +18,7 @@ package dubbo3

import (
"fmt"
tripleCommon "github.com/dubbogo/triple/pkg/common"
Copy link
Contributor

Choose a reason for hiding this comment

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

move out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

dp.serverLock.Lock()
defer dp.serverLock.Unlock()
// Stop all server
keyList := make([]string, 16)
Copy link
Contributor

Choose a reason for hiding this comment

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

move this line out of lock, pls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

@@ -201,13 +201,17 @@ func loadProviderConfig() {
checkRegistries(providerConfig.Registries, providerConfig.Registry)

for key, svs := range providerConfig.Services {
if key == "GrpcGreeterImpl2" {
fmt.Println("here")
Copy link
Member

Choose a reason for hiding this comment

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

the prints may be for your debug ??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

@AlexStocks AlexStocks merged commit ca7f3a8 into apache:3.0 Apr 3, 2021
cityiron pushed a commit that referenced this pull request Apr 11, 2021
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.

8 participants