optimize: metadata discovery support for nacos #7746
Conversation
* fix: fix client spring version compatible * fix: fix client spring version compatible * fix: fix client spring version compatible * bugfix: fix client spring version compatible * bugfix: fix client spring version compatible
…o gsoc-metadata-support-nacos
# Conflicts: # README.md # changes/en-us/2.x.md # changes/zh-cn/2.x.md # common/src/main/java/org/apache/seata/common/metadata/ServiceInstance.java # common/src/test/java/org/apache/seata/common/metadata/ServiceInstanceTest.java # core/src/main/java/org/apache/seata/core/rpc/netty/AbstractNettyRemotingClient.java # discovery/seata-discovery-consul/src/main/java/org/apache/seata/discovery/registry/consul/ConsulRegistryServiceImpl.java # discovery/seata-discovery-core/src/main/java/org/apache/seata/discovery/loadbalance/WeightedRandomLoadBalance.java # discovery/seata-discovery-etcd3/src/main/java/org/apache/seata/discovery/registry/etcd3/EtcdRegistryServiceImpl.java # discovery/seata-discovery-eureka/src/main/java/org/apache/seata/discovery/registry/eureka/EurekaRegistryServiceImpl.java # discovery/seata-discovery-nacos/src/test/java/org/apache/seata/discovery/registry/nacos/NacosRegistryServiceImplTest.java # discovery/seata-discovery-namingserver/src/main/java/org/apache/seata/discovery/registry/namingserver/NamingserverRegistryServiceImpl.java # discovery/seata-discovery-namingserver/src/test/java/org/apache/seata/discovery/registry/namingserver/NamingserverRegistryServiceImplTest.java # discovery/seata-discovery-raft/src/main/java/org/apache/seata/discovery/registry/raft/RaftRegistryServiceImpl.java # seata-spring-autoconfigure/seata-spring-autoconfigure-server/src/test/java/org/apache/seata/spring/boot/autoconfigure/properties/server/filter/ServerHttpFilterXssPropertiesTest.java # server/src/main/resources/application.raft.example.yml
There was a problem hiding this comment.
Pull Request Overview
This PR adds metadata discovery support for Nacos registry center. It enhances the Nacos registry implementation to properly handle service metadata during registration, unregistration, and subscription operations. The changes refactor the Nacos integration to use a new getNacosInstance() helper method that converts ServiceInstance objects (with metadata) to Nacos-specific Instance objects.
Key Changes:
- Refactored
NacosRegistryServiceImplto support metadata in register/unregister operations - Added
toStringMap()utility method to convert metadata fromMap<String, Object>toMap<String, String> - Comprehensive test coverage for metadata registration, unregistration, subscription, and unsubscription
- Configuration updates for test resources
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
NacosRegistryServiceImpl.java |
Refactored register/unregister to use new getNacosInstance() helper method for metadata conversion |
ServiceInstance.java |
Added toStringMap() utility to convert metadata to string map format required by Nacos |
NacosRegistryServiceImplTest.java |
Added comprehensive tests for metadata registration/discovery with integration test annotations |
registry.conf |
Updated test configuration to add cluster field and reset contextPath |
file.conf |
Added new test configuration file with service group mapping |
EnhancedServiceLoaderTest.java |
Enabled previously commented-out test method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| metadata.put("weight", 1.0); | ||
|
|
||
| ServiceInstance serviceInstance = new ServiceInstance(address, metadata); | ||
| InetSocketAddress address1 = new InetSocketAddress("127.0.0.1", 8092); |
There was a problem hiding this comment.
The test creates two ServiceInstance objects with the same address (127.0.0.1:8092). Since address on line 106 and address1 on line 112 both use port 8092, serviceInstance and serviceInstance1 are identical, which means registering both doesn't test the unregister behavior properly. One should use a different port (e.g., 8093) to properly test that unregistering one instance leaves the other registered.
| InetSocketAddress address1 = new InetSocketAddress("127.0.0.1", 8092); | |
| InetSocketAddress address1 = new InetSocketAddress("127.0.0.1", 8093); |
| } | ||
|
|
||
| List<ServiceInstance> instances = service.lookup(SERVICE_NAME); | ||
| assertTrue(instances.size() > 0); |
There was a problem hiding this comment.
[nitpick] Use assertFalse(instances.isEmpty()) or assertTrue(!instances.isEmpty()) instead of assertTrue(instances.size() > 0) for better readability and consistency with other assertions in the test file.
| assertTrue(instances.size() > 0); | |
| assertFalse(instances.isEmpty()); |
| } | ||
|
|
||
| List<ServiceInstance> instancesBefore = service.lookup(SERVICE_NAME); | ||
| assertTrue(instancesBefore.size() > 0); |
There was a problem hiding this comment.
[nitpick] Use assertFalse(instancesBefore.isEmpty()) instead of assertTrue(instancesBefore.size() > 0) for better readability and consistency with other assertions.
| assertTrue(instancesBefore.size() > 0); | |
| assertFalse(instancesBefore.isEmpty()); |
|
|
||
| // Verify unregistration success | ||
| List<ServiceInstance> instancesAfter = service.lookup(SERVICE_NAME); | ||
| assertTrue(instancesAfter.size() == 1); |
There was a problem hiding this comment.
Replace assertTrue(instancesAfter.size() == 1) with assertEquals(1, instancesAfter.size()) for clearer assertion failure messages that show the expected vs actual values.
| assertTrue(instancesAfter.size() == 1); | |
| assertEquals(1, instancesAfter.size()); |
4 participants
Ⅰ. Describe what this PR did
Enhance the Nacos registry center with metadata registration and discovery capabilities based on branch 'gsoc-2025-meta-registry'.
Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews