Re: [PR] fix(metadata): improve error message when protocol is missing [dubbo-go]
Alanxtl commented on PR #3364: URL: https://github.com/apache/dubbo-go/pull/3364#issuecomment-4608837042 Sorry, I took another look and realized this is not actually a bug in the way I originally described. The metadata service protocol is recorded in the service instance metadata, and the registry itself does not need to understand or be compatible with the metadata RPC protocol. The consumer just needs the corresponding protocol capability when it calls the provider metadata service. So this is more about sample/import setup than a framework bug. I’ll close this issue and the related PR. Thanks for looking into it and sorry for the confusion. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix(metadata): improve error message when protocol is missing [dubbo-go]
Alanxtl closed pull request #3364: fix(metadata): improve error message when protocol is missing URL: https://github.com/apache/dubbo-go/pull/3364 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix(metadata): improve error message when protocol is missing [dubbo-go]
sonarqubecloud[bot] commented on PR #3364: URL: https://github.com/apache/dubbo-go/pull/3364#issuecomment-4603742042 ## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3364) **Quality Gate passed** Issues  [1 New issue](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3364&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3364&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=3364&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3364&metric=new_coverage&view=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3364&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3364) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix(metadata): improve error message when protocol is missing [dubbo-go]
codecov-commenter commented on PR #3364: URL: https://github.com/apache/dubbo-go/pull/3364#issuecomment-4603671261 ## [Codecov](https://app.codecov.io/gh/apache/dubbo-go/pull/3364?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report :white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 52.53%. Comparing base ([`60d1c2a`](https://app.codecov.io/gh/apache/dubbo-go/commit/60d1c2a949f0ee0da4be3fe09fb79d295491e040?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`780c3e2`](https://app.codecov.io/gh/apache/dubbo-go/commit/780c3e25a264ad11394287c2eef90a74bbf7481b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)). :warning: Report is 815 commits behind head on develop. Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop#3364 +/- ## === + Coverage46.76% 52.53% +5.76% === Files 295 492 +197 Lines1717237871 +20699 === + Hits 803119896 +11865 - Misses828716369+8082 - Partials 854 1606 +752 ``` [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/dubbo-go/pull/3364?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :rocket: New features to boost your workflow: - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix(metadata): improve error message when protocol is missing [dubbo-go]
wlgusqkr commented on PR #3364:
URL: https://github.com/apache/dubbo-go/pull/3364#issuecomment-4603592208
@Alanxtl Thanks for the guidance! I've updated the PR to follow this
direction.
I reverted the `GetProtocolWithError` / metadata import-hint change and
added a side-effect import of `protocol/dubbo` in `registry/servicediscovery`,
so application-level service discovery registers its internal metadata
transport dependency automatically.
I kept the code change minimal and did not add a test in this revision. I
considered adding an isolated import test under something like
`registry/servicediscovery/importtest`, because an in-package test under
`registry/servicediscovery` can be affected by existing tests that call
`extension.SetProtocol("dubbo", ...)` with a mock protocol.
Do you think adding that isolated import test is worthwhile here, or would
you prefer keeping this as the minimal side-effect import change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] fix(metadata): improve error message when protocol is missing [dubbo-go]
sonarqubecloud[bot] commented on PR #3364: URL: https://github.com/apache/dubbo-go/pull/3364#issuecomment-4603579474 ## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3364) **Quality Gate passed** Issues  [1 New issue](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3364&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3364&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=3364&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3364&metric=new_coverage&view=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3364&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3364) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix(metadata): improve error message when protocol is missing [dubbo-go]
Copilot commented on code in PR #3364:
URL: https://github.com/apache/dubbo-go/pull/3364#discussion_r3340744025
##
metadata/client.go:
##
@@ -53,7 +53,15 @@ func GetMetadataFromMetadataReport(revision string, instance
registry.ServiceIns
func GetMetadataFromRpc(revision string, instance registry.ServiceInstance)
(*info.MetadataInfo, error) {
url := buildStandardMetadataServiceURL(instance)
url.SetParam(constant.TimeoutKey, defaultTimeout)
- p := extension.GetProtocol(url.Protocol)
+ p, err := extension.GetProtocolWithError(url.Protocol)
Review Comment:
`buildStandardMetadataServiceURL` can return nil when the instance metadata
doesn't contain a protocol (see `buildStandardMetadataServiceURL` returning nil
when `ps[constant.ProtocolKey] == ""`). `GetMetadataFromRpc` dereferences `url`
immediately (`url.SetParam(...)`), which will still panic on that path.
Consider returning a descriptive error instead so missing/invalid metadata
doesn't crash the consumer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] fix(metadata): improve error message when protocol is missing [dubbo-go]
Alanxtl commented on PR #3364: URL: https://github.com/apache/dubbo-go/pull/3364#issuecomment-4601531507 Thanks for working on this, but I think we should not fix this by improving the error message. The issue is not that the error is unclear. The issue is that REST application-level service discovery leaks an internal metadata transport dependency to user code. For a REST consumer, users configure and import REST-related components: ```go _ "dubbo.apache.org/dubbo-go/v3/protocol/rest" _ "dubbo.apache.org/dubbo-go/v3/registry/servicediscovery" ``` But application-level discovery internally fetches metadata through: ```text dubbo://.../org.apache.dubbo.metadata.MetadataService ``` So today the consumer also has to manually import: ```go _ "dubbo.apache.org/dubbo-go/v3/protocol/dubbo" ``` That should not be required. The Dubbo protocol here is an internal metadata-service transport detail. I suggest changing the fix direction: 1. Revert the new `GetProtocolWithError` / metadata-specific import hint change. 2. Make application-level service discovery register its internal metadata transport dependency automatically. 3. Add a test proving that a REST consumer using service discovery works without importing `protocol/dubbo`. A minimal fix may be to add a side-effect import in the service discovery package: ```go import ( _ "dubbo.apache.org/dubbo-go/v3/protocol/dubbo" ) ``` Probably `registry/servicediscovery` is a reasonable place, because that is the feature path that calls `metadata.GetMetadataFromRpc(...)` and requires the default Dubbo metadata protocol. The expected test should cover: - provider protocol: REST; - registry type: application-level service discovery; - consumer imports REST protocol and service discovery packages; - consumer does **not** import `dubbo.apache.org/dubbo-go/v3/protocol/dubbo`; - metadata is still fetched and the REST provider URL is reconstructed successfully. So I would prefer fixing the dependency registration instead of returning a clearer error that still asks REST users to import Dubbo manually. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix(metadata): improve error message when protocol is missing [dubbo-go]
sonarqubecloud[bot] commented on PR #3364: URL: https://github.com/apache/dubbo-go/pull/3364#issuecomment-4601462911 ## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3364) **Quality Gate passed** Issues  [0 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3364&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3364&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=3364&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3364&metric=new_coverage&view=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3364&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3364) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix(metadata): improve error message when protocol is missing [dubbo-go]
Alanxtl commented on PR #3364: URL: https://github.com/apache/dubbo-go/pull/3364#issuecomment-4601445747 our project use `import-formatter` to format import blocks, that's the reason why ur CI fails. For you, u should 1. run `go install github.com/dubbogo/tools/cmd/imports-formatter@latest` 2. cd to the root dir of `dubbo-go` 3. run `imports-formatter` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
