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

(WIP) ServiceDiscoveryRegistry implementation #491

Closed
wants to merge 141 commits into from

Conversation

lzp0412
Copy link
Contributor

@lzp0412 lzp0412 commented Apr 24, 2020

@flycash flycash changed the title Feature/dubbo 2.7.5 ServiceDiscoveryRegistry implementation Apr 26, 2020
@flycash flycash requested review from flycash and hxmhlt April 26, 2020 01:50
Copy link
Member

@flycash flycash left a comment

Choose a reason for hiding this comment

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

  1. Split imports into three blocks. You can take other files as reference.
  2. Add comments for each public method, structure, interface.
  3. Please rebase or merge the 2.7.5 branch.

common/extension/service_instance_selector_factory.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@@ -18,11 +18,11 @@
package extension

import (
"github.com/apache/dubbo-go/registry"
Copy link
Member

Choose a reason for hiding this comment

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

format

)

func SetServiceNameMapping(name string, creator func() mapping.ServiceNameMapping) {
// TODO(@邓大明)
Copy link
Member

Choose a reason for hiding this comment

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

use english pls

}

func GetServiceNameMapping(name string) mapping.ServiceNameMapping {
// TODO(@邓大明)
Copy link
Member

Choose a reason for hiding this comment

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

as above

Comment on lines 21 to 26
"github.com/apache/dubbo-go/common"
"github.com/apache/dubbo-go/common/extension"
"github.com/apache/dubbo-go/registry"
"github.com/apache/dubbo-go/registry/service/instance"
"math/rand"
"time"
Copy link
Member

Choose a reason for hiding this comment

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

split those package

Comment on lines 21 to 24
"github.com/apache/dubbo-go/common"
"github.com/apache/dubbo-go/registry"
"github.com/stretchr/testify/assert"
"testing"
Copy link
Member

Choose a reason for hiding this comment

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

format those package

Comment on lines 344 to 349
s.lock.Lock()
// 1. expunge stale
s.expungeStaleRevisionExportedURLs(serviceInstances)
// 2. Initialize
s.initRevisionExportedURLs(serviceInstances)
s.lock.Unlock()
Copy link
Member

Choose a reason for hiding this comment

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

i think this lock is too large, can shorten it?

currentRevision := gxset.NewSet()
for _, s := range serviceInstances {
rv := getExportedServicesRevision(s)
if len(rv) != 0 {
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
if len(rv) != 0 {
if len(rv) > 0 {

)

func TestServiceDiscoveryRegistry_Register(t *testing.T) {
//registryURL,_:=common.NewURL("in-memory://localhost:12345",
Copy link
Member

Choose a reason for hiding this comment

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

delete it if useless

Comment on lines 21 to 26
"github.com/apache/dubbo-go/common"
"github.com/apache/dubbo-go/common/constant"
"github.com/apache/dubbo-go/registry"
"github.com/apache/dubbo-go/registry/service/synthesizer"
"net/url"
"strings"
Copy link
Member

Choose a reason for hiding this comment

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

format

Comment on lines 21 to 26
"github.com/apache/dubbo-go/common"
"github.com/apache/dubbo-go/common/constant"
"github.com/apache/dubbo-go/registry"
"github.com/stretchr/testify/assert"
"net/url"
"testing"
Copy link
Member

Choose a reason for hiding this comment

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

format

@codecov-io
Copy link

codecov-io commented May 2, 2020

Codecov Report

Merging #491 into feature/dubbo-2.7.5 will decrease coverage by 2.28%.
The diff coverage is 22.44%.

Impacted file tree graph

@@                   Coverage Diff                   @@
##           feature/dubbo-2.7.5     #491      +/-   ##
=======================================================
- Coverage                66.81%   64.53%   -2.29%     
=======================================================
  Files                      196      202       +6     
  Lines                    10125    10596     +471     
=======================================================
+ Hits                      6765     6838      +73     
- Misses                    2708     3103     +395     
- Partials                   652      655       +3     
Impacted Files Coverage Δ
cluster/directory/base_directory.go 72.50% <ø> (+6.59%) ⬆️
common/extension/service_discovery.go 0.00% <0.00%> (ø)
...mon/extension/service_instance_selector_factory.go 0.00% <0.00%> (ø)
common/extension/service_name_mapping.go 0.00% <0.00%> (ø)
config/consumer_config.go 56.25% <ø> (ø)
config/provider_config.go 55.88% <ø> (ø)
config/reference_config.go 78.70% <ø> (ø)
config/router_config.go 0.00% <0.00%> (-85.72%) ⬇️
...try/servicediscovery/service_discovery_registry.go 0.57% <0.57%> (ø)
metadata/mapping/dynamic/service_name_mapping.go 69.23% <40.00%> (-19.01%) ⬇️
... and 21 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 6a558d8...aaeabc6. Read the comment docs.

…nto feature/dubbo-2.7.5

# Conflicts:
#	config/service_config.go
#	config/service_config_test.go
#	go.sum
svs.id = key
svs.Implement(rpcService)
if err := svs.Export(); err != nil {
panic(fmt.Sprintf("service %s export failed! err: %#v", key, err))
Copy link
Contributor

Choose a reason for hiding this comment

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

hi, new guy, pls use %+v for error to handle nil-error or self-defined error.

@@ -113,6 +113,24 @@ func NewServiceConfig(id string, context context.Context) *ServiceConfig {
}
}

// Get Random Port
func getRandomPort(protocolConfigs []*ProtocolConfig) *list.List {
Copy link
Contributor

Choose a reason for hiding this comment

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

this work has been done by Mr Zouyx. why u do it again?

Comment on lines 184 to 186
extension.SetAndInitGlobalDispatcher(providerConfig.eventDispatcherType)

extension.SetAndInitGlobalDispatcher(consumerConfig.eventDispatcherType)
Copy link
Member

Choose a reason for hiding this comment

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

the same to line 184?

Comment on lines 105 to 107
extension.SetAndInitGlobalDispatcher(consumerConfig.eventDispatcherType)

extension.SetAndInitGlobalDispatcher(consumerConfig.eventDispatcherType)
Copy link
Member

Choose a reason for hiding this comment

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

the same to line 105?

return e.ServiceName == sicl.ServiceName
}

type ChangedNotify interface {
Copy link
Member

Choose a reason for hiding this comment

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

should it move to parent package?

Comment on lines 24 to 31
func SetMetadataServiceProxy(name string, creator func() BaseMetadataServiceProxy) {
//TODO
}

func GetMetadataServiceProxy(name string) BaseMetadataServiceProxy {
//TODO
return nil
}
Copy link
Member

Choose a reason for hiding this comment

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

should implement?


//IsAvailable for make sure is't available
func (s *serviceDiscoveryRegistry) IsAvailable() bool {
return true
Copy link
Member

Choose a reason for hiding this comment

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

always true?

flycash and others added 3 commits May 29, 2020 22:02
@codecov-commenter
Copy link

codecov-commenter commented May 30, 2020

Codecov Report

Merging #491 into feature/dubbo-2.7.5 will decrease coverage by 2.42%.
The diff coverage is 30.89%.

Impacted file tree graph

@@                   Coverage Diff                   @@
##           feature/dubbo-2.7.5     #491      +/-   ##
=======================================================
- Coverage                66.06%   63.63%   -2.43%     
=======================================================
  Files                      203      210       +7     
  Lines                    10558    11163     +605     
=======================================================
+ Hits                      6975     7104     +129     
- Misses                    2909     3371     +462     
- Partials                   674      688      +14     
Impacted Files Coverage Δ
cluster/directory/base_directory.go 72.50% <ø> (+6.59%) ⬆️
common/extension/service_discovery.go 0.00% <0.00%> (ø)
...mon/extension/service_instance_selector_factory.go 0.00% <0.00%> (ø)
common/extension/service_name_mapping.go 0.00% <0.00%> (ø)
config/consumer_config.go 56.25% <ø> (ø)
config/provider_config.go 55.88% <ø> (ø)
config/reference_config.go 78.70% <ø> (ø)
config/router_config.go 0.00% <0.00%> (-85.72%) ⬇️
metadata/mapping/dynamic/service_name_mapping.go 61.90% <0.00%> (-26.34%) ⬇️
...try/servicediscovery/service_discovery_registry.go 0.56% <0.56%> (ø)
... and 26 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 9578fc0...7019888. Read the comment docs.

})
}

// StoreConsumerMetadata will store the metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

store the consumer metadata.

client config_client.IConfigClient
}

// StoreProviderMetadata will store the metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

store the provider metadata

})
}

// SaveServiceMetadata will store the metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

save the service metadata.

}

bytes, err := json.Marshal(urlStrList)

Copy link
Contributor

Choose a reason for hiding this comment

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

delete this blank line.


package nacos

import (
Copy link
Contributor

Choose a reason for hiding this comment

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

split.

if err != nil {
return nil, err
}
clientConfig.TimeoutMs = uint64(timeout.Seconds() * 1000)
Copy link
Contributor

Choose a reason for hiding this comment

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

pls init the config in a struct.

   clientConfig := nacosConstant.ClientConfig {
       xxx: xxx,
   }

@flycash flycash linked an issue Jun 7, 2020 that may be closed by this pull request
@flycash flycash closed this Jun 13, 2020
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.

Align 2.7.5: ServiceDiscoveryRegistry