-
Notifications
You must be signed in to change notification settings - Fork 26.4k
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
Fix the problem of multiple namespaces in MetadataReport #13971
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 3.2 #13971 +/- ##
==========================================
- Coverage 70.41% 70.25% -0.17%
==========================================
Files 1607 1607
Lines 70094 70101 +7
Branches 10102 10105 +3
==========================================
- Hits 49360 49251 -109
- Misses 16088 16216 +128
+ Partials 4646 4634 -12 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chickenlj PTAL
I think it would be better to just change the way how For now, Then the only place we need to change except for the |
Ok, I will try it. |
@chickenlj PTAL |
return URL.valueOf(url.toServiceString()) | ||
.addParameter(NAMESPACE_KEY, namespace) | ||
.toString(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you describe the difference between the new url and the old url?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about making it a public util method to make calling it in other places easier? like here https://github.com/apache/dubbo/pull/13971/files#diff-17a0f234d88728ebc6f37959d84276704ca16846cd10cd0743616a8689949eabR33
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dubbo-common/src/main/java/org/apache/dubbo/config/RegistryConfig.java
Outdated
Show resolved
Hide resolved
@chickenlj Does DynamicConfiguration need to support multiple namespaces? |
Sorry for the late response, I am working on this pull request now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@finefuture thanks for the patience and contribution, we do appreciate it. |
|
What is the purpose of the change
Key missing namespace information:
![image](https://private-user-images.githubusercontent.com/18413695/314920326-fde4b37e-278c-458f-8dcf-d7bafe871575.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQyOTUwMDgsIm5iZiI6MTcyNDI5NDcwOCwicGF0aCI6Ii8xODQxMzY5NS8zMTQ5MjAzMjYtZmRlNGIzN2UtMjc4Yy00NThmLThkY2YtZDdiYWZlODcxNTc1LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjIlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODIyVDAyNDUwOFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWFhYWI1Y2UxMmE2NWE0ZDcyYzlkYjkzN2Y2ZTRmM2FlM2VjOGJiNzBhYjg3MzE5NWNhZmEwZTAzN2NjYTczY2YmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.usUuqDVXTMLfhtQObOV-ORVhQdK1aEF3dcKOYGe7s1c)
Brief changelog
Verifying this change
Checklist