Ftr: zk/consul Metadata #633

gaoxinge · 2020-06-26T15:21:17Z

What this PR does:

add consul metadata report
add zookeeper metadata report

Which issue(s) this PR fixes:

Fixes #445.

Special notes for your reviewer:

TODO
- add consul metadata report unit test
- add zookeeper metadata report unit test

Does this PR introduce a user-facing change?:

NONE

codecov-commenter · 2020-06-28T07:32:45Z

Codecov Report

Merging #633 into feature/dubbo-2.7.5 will decrease coverage by 0.06%.
The diff coverage is 72.84%.

@@                   Coverage Diff                   @@
##           feature/dubbo-2.7.5     #633      +/-   ##
=======================================================
- Coverage                65.98%   65.92%   -0.07%     
=======================================================
  Files                      204      207       +3     
  Lines                    10604    10722     +118     
=======================================================
+ Hits                      6997     7068      +71     
- Misses                    2929     2972      +43     
- Partials                   678      682       +4

Impacted Files	Coverage Δ
metadata/report/delegate/delegate_report.go	`29.41% <0.00%> (-19.82%)`	⬇️
metadata/report/nacos/report.go	`72.09% <68.75%> (+1.88%)`	⬆️
metadata/report/consul/report.go	`75.51% <75.51%> (ø)`
remoting/consul/test_agent.go	`81.81% <81.81%> (ø)`
metadata/report/zookeeper/report.go	`82.00% <82.00%> (ø)`
remoting/zookeeper/client.go	`66.85% <100.00%> (+0.19%)`	⬆️
registry/nacos/listener.go	`78.18% <0.00%> (-1.82%)`	⬇️
... and 3 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 024f7b2...302dbc2. Read the comment docs.

.gitignore

metadata/report/consul/report.go

metadata/report/nacos/report.go

metadata/report/consul/report.go

hxmhlt · 2020-06-29T02:14:05Z

metadata/report/consul/report.go

+
+// StoreProviderMetadata stores the metadata.
+func (m *consulMetadataReport) StoreProviderMetadata(providerIdentifier *identifier.MetadataIdentifier, serviceDefinitions string) error {
+	kv := &consul.KVPair{Key: providerIdentifier.GetIdentifierKey(), Value: []byte(serviceDefinitions)}


go fmt not work ?

It is ok with go fmt.

hxmhlt · 2020-06-29T02:14:55Z

metadata/report/consul/report.go

+	k := metadataIdentifier.GetIdentifierKey()
+	kv, _, err := m.client.KV().Get(k, nil)
+	if err != nil {
+		panic(err)


don't panic

I remove this panic and return an error in this function.

hxmhlt · 2020-06-29T02:16:26Z

metadata/report/delegate/delegate_report.go

+	if err != nil {
+		return perrors.WithMessage(err, "Could not convert the array to json")
+	}
+
 	report := instance.GetMetadataReportInstance()


This common logic may be confirmed by @flycash

May create influence to nacos's implement.

I think this is a common code for different metadata reports, and one can find the similar code in java version.

Let's keep those codes, I will test it again

metadata/report/nacos/report.go

metadata/report/zookeeper/report.go

hxmhlt · 2020-06-29T02:23:49Z

remoting/consul/agent.go

@@ -0,0 +1,70 @@
+/*


What the consultAgent used for ?

If used for unit test, it's better to move to xxx_test file.

This consulAgent is used for test, and is imported by other test, so I rename agent.go to test_agent.go. (xxx_test.go can not be imported by other test.)

metadata/report/consul/report.go

AlexStocks · 2020-07-01T05:04:49Z

metadata/report/consul/report.go

+// GetSubscribedURLs gets the urls.
+func (m *consulMetadataReport) GetSubscribedURLs(subscriberMetadataIdentifier *identifier.SubscriberMetadataIdentifier) ([]string, error) {
+	k := subscriberMetadataIdentifier.GetIdentifierKey()
+	kv, _, err := m.client.KV().Get(k, nil)


if err != nil || kv == nil {
return emptyStrSlice, err
}

I follow your advice and keep code more simpler.

metadata/report/consul/report.go

flycash · 2020-07-01T12:03:13Z

metadata/report/delegate/delegate_report.go

+	if err != nil {
+		return perrors.WithMessage(err, "Could not convert the array to json")
+	}
+
 	report := instance.GetMetadataReportInstance()


Let's keep those codes, I will test it again

metadata/report/nacos/report.go

metadata/report/zookeeper/report.go

flycash · 2020-07-01T12:08:53Z

metadata/report/zookeeper/report.go

+func (m *zookeeperMetadataReport) GetSubscribedURLs(subscriberMetadataIdentifier *identifier.SubscriberMetadataIdentifier) ([]string, error) {
+	k := m.rootDir + subscriberMetadataIdentifier.GetFilePathKey()
+	v, _, err := m.client.GetContent(k)
+	if err != nil {


see above comment

I follow your advice and keep code more simpler.

metadata/report/consul/report_test.go

metadata/report/report.go

metadata/report/zookeeper/report_test.go

pantianying

LGTM

xg.gao added 7 commits June 26, 2020 13:22

modify metareport interface

3d0b1c9

fix bug

b8f5346

consul metadata report

b2a04fd

zookeeper metadata report

117f85f

fix

f1576d1

fix

242e9f2

import sort

81a458c

AlexStocks changed the title ~~WIP: Metadata~~ WIP: zk Metadata Jun 27, 2020

AlexStocks changed the title ~~WIP: zk Metadata~~ WIP: Metadata Jun 27, 2020

AlexStocks changed the title ~~WIP: Metadata~~ WIP: zk/consul Metadata Jun 27, 2020

xg.gao added 2 commits June 27, 2020 21:59

consul metadata report unit test

404cd00

zookeeper metadata report unit test

f004237

go fmt

ecf7fa1

gaoxinge changed the title ~~WIP: zk/consul Metadata~~ Ftr: zk/consul Metadata Jun 28, 2020

zouyx reviewed Jun 28, 2020

View reviewed changes

.gitignore Show resolved Hide resolved

metadata/report/consul/report.go Outdated Show resolved Hide resolved

metadata/report/consul/report.go Outdated Show resolved Hide resolved

metadata/report/nacos/report.go Show resolved Hide resolved

xg.gao added 2 commits June 28, 2020 19:28

global var for empty slice

44dbdfe

fix

f440a31

hxmhlt reviewed Jun 29, 2020

View reviewed changes

xg.gao added 3 commits June 29, 2020 12:30

remove panic

e030e25

fix bug

a601a33

rename consul agent for test

f00d104

AlexStocks approved these changes Jun 29, 2020

View reviewed changes

AlexStocks reviewed Jul 1, 2020

View reviewed changes

metadata/report/consul/report.go Outdated Show resolved Hide resolved

AlexStocks reviewed Jul 1, 2020

View reviewed changes

flycash reviewed Jul 1, 2020

View reviewed changes

xg.gao added 3 commits July 1, 2020 21:02

fix

a23c6d2

fix typo

7248544

update zk library version

f66dda8

zouyx reviewed Jul 1, 2020

View reviewed changes

metadata/report/consul/report_test.go Outdated Show resolved Hide resolved

metadata/report/report.go Outdated Show resolved Hide resolved

metadata/report/zookeeper/report_test.go Outdated Show resolved Hide resolved

simplify and comment

302dbc2

flycash approved these changes Jul 3, 2020

View reviewed changes

pantianying approved these changes Jul 4, 2020

View reviewed changes

pantianying merged commit 5a12137 into apache:feature/dubbo-2.7.5 Jul 4, 2020

gaoxinge deleted the metadata branch July 5, 2020 02:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ftr: zk/consul Metadata #633

Ftr: zk/consul Metadata #633

gaoxinge commented Jun 26, 2020 •

edited

Loading

codecov-commenter commented Jun 28, 2020 •

edited

Loading

hxmhlt Jun 29, 2020

gaoxinge Jun 29, 2020

hxmhlt Jun 29, 2020

gaoxinge Jun 29, 2020

hxmhlt Jun 29, 2020

hxmhlt Jun 29, 2020

gaoxinge Jun 29, 2020 •

edited

Loading

flycash Jul 1, 2020

hxmhlt Jun 29, 2020

hxmhlt Jun 29, 2020

gaoxinge Jun 29, 2020 •

edited

Loading

AlexStocks Jul 1, 2020

gaoxinge Jul 1, 2020

flycash Jul 1, 2020

flycash Jul 1, 2020

gaoxinge Jul 1, 2020

pantianying left a comment

Ftr: zk/consul Metadata #633

Ftr: zk/consul Metadata #633

Conversation

gaoxinge commented Jun 26, 2020 • edited Loading

codecov-commenter commented Jun 28, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaoxinge Jun 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaoxinge Jun 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pantianying left a comment

Choose a reason for hiding this comment

gaoxinge commented Jun 26, 2020 •

edited

Loading

codecov-commenter commented Jun 28, 2020 •

edited

Loading

gaoxinge Jun 29, 2020 •

edited

Loading

gaoxinge Jun 29, 2020 •

edited

Loading