Add: DelegateMetadataReport & RemoteMetadataService #505

hxmhlt · 2020-05-06T09:53:49Z

What this PR does:

1.Finish delegate metadata report as all metadata report implements father.
2.Finish remote metadata service to implement MetadataService interface.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

metadata/report/delegate/delegate_report.go

flycash

It seems that you convert the MetadataReportConfig to URL, and then read params from URL.So why don't you use MetadataReportConfig directly?

flycash · 2020-05-07T06:45:02Z

metadata/report/delegate/delegate_report.go

+
+// metadataReportRetry is a scheduler for retrying task
+type metadataReportRetry struct {
+	retryPeriod  int64


Using duration would be better

it is used by go-co-op/gocron. The param type is int64.

config/instance/metedata_report.go

metadata/service/inmemory/service.go

metadata/service/remote/service.go

metadata/service/service.go

metadata/definition/definition.go

metadata/report/delegate/delegate_report.go

metadata/report/delegate/delegate_report_test.go

metadata/definition/definition.go

metadata/report/delegate/delegate_report_test.go

metadata/service/remote/service.go

…ta_report

codecov-io · 2020-05-17T08:34:39Z

Codecov Report

Merging #505 into feature/dubbo-2.7.5 will decrease coverage by 0.69%.
The diff coverage is 43.70%.

@@                   Coverage Diff                   @@
##           feature/dubbo-2.7.5     #505      +/-   ##
=======================================================
- Coverage                66.66%   65.96%   -0.70%     
=======================================================
  Files                      199      203       +4     
  Lines                    10212    10487     +275     
=======================================================
+ Hits                      6808     6918     +110     
- Misses                    2751     2901     +150     
- Partials                   653      668      +15

Impacted Files	Coverage Δ
metadata/identifier/service_metadata_identifier.go	`30.76% <0.00%> (-69.24%)`	⬇️
...tadata/identifier/subscribe_metadata_identifier.go	`100.00% <ø> (ø)`
metadata/definition/mock.go	`28.57% <28.57%> (ø)`
metadata/report/delegate/delegate_report.go	`41.08% <41.08%> (ø)`
metadata/service/remote/service.go	`43.95% <43.95%> (ø)`
metadata/definition/definition.go	`58.97% <62.50%> (ø)`
metadata/service/inmemory/service.go	`87.03% <70.58%> (ø)`
metadata/service/exporter/configurable/exporter.go	`94.59% <100.00%> (ø)`
config_center/nacos/facade.go	`37.50% <0.00%> (-25.00%)`	⬇️
config_center/nacos/client.go	`55.55% <0.00%> (-1.71%)`	⬇️
... and 6 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 c958389...235edf5. Read the comment docs.

metadata/report/delegate/delegate_report.go

zouyx · 2020-05-17T14:00:08Z

metadata/definition/mock.go

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


is it just used for test case?

I do not think it is a problem. Dubbo Java has many similar practices.

If just for test ,then i think that is a problem, because it can be use out of test case .

what do you think ? @flycash @fangyincheng @AlexStocks

metadata/definition/definition.go

micln · 2020-05-17T14:29:32Z

metadata/definition/definition.go

+	for _, m := range def.Methods {
+		var paramType strings.Builder
+		for _, p := range m.ParameterTypes {
+			paramType.WriteString(fmt.Sprintf("{type:%v}", p))


fmt.Fprintf(paramType, "{type:%v}", p) ?

if you wanna got its type, u should use %T.

if you wanna got its type, u should use %T.

type is just the variable's name. Not reflect.type meaning.

micln · 2020-05-17T14:34:07Z

metadata/report/delegate/delegate_report.go

+
+// newMetadataReportRetry will create a scheduler for retry task
+func newMetadataReportRetry(retryPeriod int64, retryLimit int64, retryFunc func() bool) (*metadataReportRetry, error) {
+	s1 := gocron.NewScheduler(time.UTC)


dost UTC need be set by users

It does not involve specific time, but only requires the user to set interval time, so there is no problem of time zone difference.

codecov-commenter · 2020-05-23T15:56:30Z

Codecov Report

Merging #505 into feature/dubbo-2.7.5 will decrease coverage by 0.81%.
The diff coverage is 39.19%.

@@                   Coverage Diff                   @@
##           feature/dubbo-2.7.5     #505      +/-   ##
=======================================================
- Coverage                66.66%   65.85%   -0.82%     
=======================================================
  Files                      199      203       +4     
  Lines                    10212    10558     +346     
=======================================================
+ Hits                      6808     6953     +145     
- Misses                    2751     2935     +184     
- Partials                   653      670      +17

Impacted Files	Coverage Δ
metadata/identifier/service_metadata_identifier.go	`30.76% <0.00%> (-69.24%)`	⬇️
...tadata/identifier/subscribe_metadata_identifier.go	`100.00% <ø> (ø)`
metadata/definition/mock.go	`28.57% <28.57%> (ø)`
metadata/report/delegate/delegate_report.go	`30.76% <30.76%> (ø)`
metadata/service/remote/service.go	`43.95% <43.95%> (ø)`
metadata/definition/definition.go	`58.97% <66.66%> (ø)`
metadata/service/inmemory/service.go	`87.03% <70.58%> (ø)`
metadata/service/exporter/configurable/exporter.go	`94.59% <100.00%> (ø)`
cluster/cluster_impl/base_cluster_invoker.go	`62.31% <0.00%> (-10.15%)`	⬇️
registry/etcdv3/registry.go	`69.01% <0.00%> (-4.13%)`	⬇️
... and 12 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 c958389...c024ce1. Read the comment docs.

gaoxinge · 2020-05-24T03:43:28Z

Nice job! LGTM.

AlexStocks · 2020-05-24T10:48:19Z

go.sum

@@ -153,6 +151,7 @@ github.com/go-openapi/jsonpointer v0.0.0-20160704185906-46af16f9f7b1/go.mod h1:+
 github.com/go-openapi/jsonreference v0.0.0-20160704190145-13c6e3589ad9/go.mod h1:W3Z9FmVs9qj+KR4zFKmDPGiLdk1D9Rlm7cyMvf57TTg=
 github.com/go-openapi/spec v0.0.0-20160808142527-6aced65f8501/go.mod h1:J8+jY1nAiCcj+friV/PDoE1/3eeccG9LYBs0tYvLOWc=
 github.com/go-openapi/swag v0.0.0-20160704191624-1d0bd113de87/go.mod h1:DXUve3Dpr1UfpPtxFw+EFuQ41HhCWZfha5jSVRG7C7I=
+github.com/go-redis/redis v6.15.5+incompatible/go.mod h1:NAIEuMOZ/fxfXJIrKDQDz8wamY7mA7PouImQ2Jvg6kA=


why import redis?

I import go-cron lib and redis lib is imported.

AlexStocks · 2020-05-24T10:49:12Z

metadata/definition/definition.go

+	for _, m := range def.Methods {
+		var paramType strings.Builder
+		for _, p := range m.ParameterTypes {
+			paramType.WriteString(fmt.Sprintf("{type:%v}", p))


if you wanna got its type, u should use %T.

AlexStocks · 2020-05-24T10:51:02Z

metadata/report/delegate/delegate_report.go

+		scheduler := gocron.NewScheduler(time.UTC)
+		_, err := scheduler.Every(1).Day().Do(
+			func() {
+				logger.Info("start to publish all metadata.")


it is nonsense that writing a log without its context.

type is just the variable's name. Not reflect.type meaning.

it is nonsense that writing a log without its context.

done

metadata/report/delegate/delegate_report.go

AlexStocks · 2020-05-24T10:58:49Z

metadata/report/delegate/delegate_report.go

+	if bmr.syncReport {
+		return report.SaveServiceMetadata(identifier, url)
+	}
+	go report.SaveServiceMetadata(identifier, url)


pls using gosafely in gost (https://github.com/dubbogo/gost/blob/8b727f60a1cb164b3e7797c7d3c78143771a4e70/runtime/goroutine.go#L29)

I think the panic should be recover by the func called. Because we don't know what should we do because SaveServiceMetadata method is provided by extension metadata like ZooKeeper, etcd and so on. They should provider their error handler method.

AlexStocks · 2020-05-24T10:58:59Z

metadata/report/delegate/delegate_report.go

+	if bmr.syncReport {
+		return report.RemoveServiceMetadata(identifier)
+	}
+	go report.RemoveServiceMetadata(identifier)


pls using gosafely in gost

AlexStocks · 2020-05-24T10:59:04Z

metadata/report/delegate/delegate_report.go

+	if bmr.syncReport {
+		return report.SaveSubscribedData(identifier, urls)
+	}
+	go report.SaveSubscribedData(identifier, urls)


pls using gosafely in gost

metadata/service/remote/service.go

hxmhlt added 3 commits May 6, 2020 17:44

Add:metadata report delegate & remote metadata service

b87dbe2

Mod:for ut

73f21fa

Mod:resolve conflict

f7dd531

hxmhlt requested review from flycash, AlexStocks, fangyincheng and zouyx May 6, 2020 10:07

zouyx reviewed May 6, 2020

View reviewed changes

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

flycash reviewed May 7, 2020

View reviewed changes

flycash approved these changes May 11, 2020

View reviewed changes

fangyincheng reviewed May 11, 2020

View reviewed changes

config/instance/metedata_report.go Outdated Show resolved Hide resolved

metadata/service/inmemory/service.go Outdated Show resolved Hide resolved

metadata/service/remote/service.go Outdated Show resolved Hide resolved

zouyx reviewed May 12, 2020

View reviewed changes

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

gaoxinge reviewed May 12, 2020

View reviewed changes

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

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

metadata/service/remote/service.go Outdated Show resolved Hide resolved

hxmhlt added 2 commits May 17, 2020 16:00

Merge remote-tracking branch 'apache/feature/dubbo-2.7.5' into metada…

9788244

…ta_report

Mod:resolve pr review

0048609

AlexStocks reviewed May 17, 2020

View reviewed changes

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

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

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

Mod:resolve pr review

235edf5

zouyx reviewed May 17, 2020

View reviewed changes

micln reviewed May 17, 2020

View reviewed changes

Mod:modify for code review

c312cc8

Mod:for code review

0d78b2a

AlexStocks reviewed May 24, 2020

View reviewed changes

Mod:code review

c024ce1

micln approved these changes May 24, 2020

View reviewed changes

AlexStocks approved these changes May 25, 2020

View reviewed changes

AlexStocks merged commit d4c83fb into apache:feature/dubbo-2.7.5 May 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add: DelegateMetadataReport & RemoteMetadataService #505

Add: DelegateMetadataReport & RemoteMetadataService #505

hxmhlt commented May 6, 2020

flycash left a comment

flycash May 7, 2020

hxmhlt May 17, 2020

codecov-io commented May 17, 2020 •

edited

Loading

zouyx May 17, 2020

hxmhlt May 23, 2020

hxmhlt May 23, 2020

zouyx May 24, 2020

micln May 17, 2020

hxmhlt May 23, 2020

AlexStocks May 24, 2020

hxmhlt May 24, 2020

micln May 17, 2020

hxmhlt May 23, 2020

codecov-commenter commented May 23, 2020 •

edited

Loading

gaoxinge commented May 24, 2020

AlexStocks May 24, 2020

hxmhlt May 24, 2020

AlexStocks May 24, 2020

AlexStocks May 24, 2020

hxmhlt May 24, 2020

hxmhlt May 24, 2020

AlexStocks May 24, 2020

hxmhlt May 24, 2020 •

edited

Loading

AlexStocks May 24, 2020

hxmhlt May 24, 2020

AlexStocks May 24, 2020

hxmhlt May 24, 2020

Add: DelegateMetadataReport & RemoteMetadataService #505

Add: DelegateMetadataReport & RemoteMetadataService #505

Conversation

hxmhlt commented May 6, 2020

flycash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented May 17, 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

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

codecov-commenter commented May 23, 2020 • edited Loading

Codecov Report

gaoxinge commented May 24, 2020

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

Choose a reason for hiding this comment

hxmhlt May 24, 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

codecov-io commented May 17, 2020 •

edited

Loading

codecov-commenter commented May 23, 2020 •

edited

Loading

hxmhlt May 24, 2020 •

edited

Loading