Mesh Registry support custom group for sofa registry. #1454
Conversation
WalkthroughThe changes enhance the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (12)
registry/registry-mesh/src/main/java/com/alipay/sofa/rpc/registry/mesh/model/UnPublishServiceRequest.java (1)
Line range hint
57-64: UpdatetoString()method to include thegroupfieldThe
toString()method should be updated to include the newgroupfield for completeness and consistency in object representation. This will ensure that all relevant information is available when logging or debugging.Consider updating the
toString()method as follows:@Override public String toString() { final StringBuffer sb = new StringBuffer("UnPublishServiceRequest{"); sb.append("serviceName='").append(serviceName).append('\''); sb.append(", protocolType='").append(protocolType).append('\''); + sb.append(", group='").append(group).append('\''); sb.append('}'); return sb.toString(); }registry/registry-mesh/src/main/java/com/alipay/sofa/rpc/registry/mesh/model/UnSubscribeServiceRequest.java (1)
73-73: Approved with suggestion: Update totoString()methodThe
toString()method has been correctly updated to include the newgroupfield, maintaining consistency with the existing implementation. However, to improve null safety, consider usingObjects.toString(group, "")instead of directly appendinggroup.Here's a suggested improvement:
- sb.append(", group='").append(group).append('\''); + sb.append(", group='").append(Objects.toString(group, "")).append('\'');This change ensures that if
groupis null, an empty string will be used instead, preventing potential null pointer exceptions.registry/registry-mesh/src/main/java/com/alipay/sofa/rpc/registry/mesh/model/PublishServiceRequest.java (1)
73-76: LGTM with suggestion: Addition ofsetGroup(String group)methodThe
setGroup(String group)method is correctly implemented, following Java naming conventions for setter methods. It provides appropriate public access to modify thegroupfield value.Consider adding input validation for the
groupparameter. For example:public void setGroup(String group) { if (group == null || group.trim().isEmpty()) { throw new IllegalArgumentException("Group cannot be null or empty"); } this.group = group; }This ensures that the
groupfield is always set to a meaningful value.registry/registry-mesh/src/main/java/com/alipay/sofa/rpc/registry/mesh/model/SubscribeServiceRequest.java (3)
35-43: New fields added correctly, consider adding Javadoc comments.The new fields
vipOnly,localCloudFirst,group, andpropertiesenhance the functionality of theSubscribeServiceRequestclass. The types are appropriate, and thepropertiesfield allows for extensibility.Consider adding Javadoc comments for each new field to explain their purpose and usage, especially for
vipOnly,localCloudFirst, andgroup. This will improve code documentation and maintainability.
101-107: Getter and setter forpropertiesimplemented correctly, consider defensive copying.The
getProperties()andsetProperties(Map<String, String> properties)methods are correctly implemented. However, to prevent external modification of the internal state, consider using defensive copying.Consider implementing defensive copying for the
propertiesfield:public Map<String, String> getProperties() { return properties == null ? null : new HashMap<>(properties); } public void setProperties(Map<String, String> properties) { this.properties = properties == null ? null : new HashMap<>(properties); }This approach prevents external code from modifying the internal state of the object.
118-118: Update totoString()method is correct, consider includingproperties.The
groupfield has been correctly added to the string representation. However, thepropertiesfield is not included.Consider adding the
propertiesfield to thetoString()method for completeness:sb.append(", group='").append(group).append('\''); sb.append(", properties=").append(properties); sb.append('}');This will provide a more comprehensive string representation of the object.
registry/registry-mesh/src/test/java/com/alipay/sofa/rpc/registry/mesh/MeshRegistryTest.java (3)
170-171: LGTM! Consider using a constant for the group value.The addition of the
parametermap with the SOFA_GROUP_KEY is consistent with the PR objective of adding support for custom groups.Consider defining "SOFA_TEST" as a constant at the class level for better maintainability and to avoid magic strings in the test code.
224-225: LGTM! Comprehensive verification of SubscribeServiceRequest.The added assertions correctly verify the group information and the presence of properties in the SubscribeServiceRequest. This ensures the custom group support is working as expected for subscriptions.
Consider adding a more specific assertion for the properties, such as checking for the presence of the SOFA_GROUP_KEY in the properties map. This would provide a more thorough verification of the group information propagation.
273-273: LGTM! Verification of group in UnSubscribeServiceRequest.The added assertion correctly verifies that the group information ("SOFA_TEST") is set in the UnSubscribeServiceRequest, ensuring the custom group support is maintained during service unsubscription.
To complete the test coverage, consider adding similar group assertions for the batch operations (batchUnRegister and batchUnSubscribe) at the end of the test method. This would ensure that the group information is correctly handled in bulk operations as well.
registry/registry-mesh/src/main/java/com/alipay/sofa/rpc/registry/mesh/MeshRegistry.java (3)
188-191: LGTM: Group support added to PublishServiceRequestThe changes correctly implement group support for the publish service request. Good use of
StringUtils.isNotBlank()to avoid potential null or empty string issues.Consider extracting
SofaRegistryConstants.SOFA_GROUP_KEYto a local constant in this class if it's used frequently, to improve readability and maintainability.
385-388: LGTM: Group support added to UnSubscribeServiceRequestThe changes correctly implement group support for the unsubscribe service request, consistent with the
buildSubscribeServiceRequestmethod.Consider refactoring the group extraction and setting logic into a separate method to reduce code duplication across these four methods. This would improve maintainability and ensure consistent behavior.
Line range hint
1-413: Summary: Group support successfully implemented with minor suggestionsThe changes successfully implement custom group support for the sofa registry across publish, unpublish, subscribe, and unsubscribe operations. The implementation is consistent and uses appropriate null/blank checks.
Next steps:
- Verify the points raised in the individual comments, particularly the different methods used to retrieve the group value in provider vs. consumer contexts.
- Consider refactoring the group extraction and setting logic into a separate method to reduce code duplication.
- If
SofaRegistryConstants.SOFA_GROUP_KEYis used frequently in this class, consider extracting it to a local constant for improved readability.Once these points are addressed, the changes will be ready for merging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- registry/registry-mesh/src/main/java/com/alipay/sofa/rpc/registry/mesh/MeshRegistry.java (5 hunks)
- registry/registry-mesh/src/main/java/com/alipay/sofa/rpc/registry/mesh/model/PublishServiceRequest.java (2 hunks)
- registry/registry-mesh/src/main/java/com/alipay/sofa/rpc/registry/mesh/model/SubscribeServiceRequest.java (3 hunks)
- registry/registry-mesh/src/main/java/com/alipay/sofa/rpc/registry/mesh/model/UnPublishServiceRequest.java (2 hunks)
- registry/registry-mesh/src/main/java/com/alipay/sofa/rpc/registry/mesh/model/UnSubscribeServiceRequest.java (2 hunks)
- registry/registry-mesh/src/test/java/com/alipay/sofa/rpc/registry/mesh/MeshRegistryTest.java (8 hunks)
🧰 Additional context used
🔇 Additional comments (23)
registry/registry-mesh/src/main/java/com/alipay/sofa/rpc/registry/mesh/model/UnPublishServiceRequest.java (3)
30-31: LGTM: Addition ofgroupfieldThe new
groupfield aligns with the PR objective of supporting custom groups for the sofa registry. It's correctly declared as private, maintaining proper encapsulation.
48-51: LGTM: Addition ofgetGroup()methodThe
getGroup()method is correctly implemented, following Java conventions and providing appropriate read access to the privategroupfield.
52-55: LGTM: Addition ofsetGroup(String group)methodThe
setGroup(String group)method is correctly implemented, following Java conventions and providing appropriate write access to the privategroupfield.registry/registry-mesh/src/main/java/com/alipay/sofa/rpc/registry/mesh/model/UnSubscribeServiceRequest.java (4)
33-34: LGTM: Addition ofgroupfieldThe new
groupfield aligns with the PR objective to support custom groups for the sofa registry. It's correctly declared as private and appropriately placed within the class structure.
59-62: LGTM: Addition ofgetGroup()methodThe
getGroup()method is correctly implemented, following Java conventions for naming and accessibility. It's appropriately placed within the class structure.
63-66: LGTM: Addition ofsetGroup(String group)methodThe
setGroup(String group)method is correctly implemented, following Java conventions for naming and accessibility. It's appropriately placed within the class structure.
Line range hint
33-73: Overall assessment: Well-implemented changes supporting custom groupsThe changes to
UnSubscribeServiceRequest.javaeffectively implement support for custom groups in the sofa registry. The newgroupfield and its accessor methods are correctly implemented and integrated into the existing class structure. These changes enhance the flexibility of the unsubscribe service request, allowing for more granular control over service grouping.The modifications follow Java conventions and best practices, maintaining code quality and readability. The only minor suggestion is to improve null safety in the
toString()method, as mentioned in the previous comment.These changes align well with the PR objective and should positively impact the functionality of the Mesh Registry.
registry/registry-mesh/src/main/java/com/alipay/sofa/rpc/registry/mesh/model/PublishServiceRequest.java (4)
35-36: LGTM: Addition ofgroupfieldThe new
groupfield aligns with the PR objective to support custom groups for the sofa registry. It's correctly declared as private, maintaining proper encapsulation.
69-72: LGTM: Addition ofgetGroup()methodThe
getGroup()method is correctly implemented, following Java naming conventions for getter methods. It provides appropriate public access to thegroupfield value.
84-84: LGTM: Update totoString()methodThe
toString()method has been correctly updated to include the newgroupfield. The format is consistent with the existing implementation, maintaining readability and completeness of the object's string representation.
Line range hint
1-87: Overall assessment: Changes align with PR objectivesThe addition of the
groupfield and its associated methods (getGroup(),setGroup(String group)) to thePublishServiceRequestclass successfully implements support for custom groups in the sofa registry. The changes are well-integrated into the existing class structure and follow good coding practices.A minor suggestion for input validation in the
setGroupmethod has been made to enhance robustness. Otherwise, the implementation is sound and ready for integration.registry/registry-mesh/src/main/java/com/alipay/sofa/rpc/registry/mesh/model/SubscribeServiceRequest.java (3)
19-20: LGTM: Import statement added correctly.The import for
java.util.Mapis necessary for the newpropertiesfield and is correctly placed in the import section.
93-99: LGTM: Getter and setter forgroupimplemented correctly.The
getGroup()andsetGroup(String group)methods are correctly implemented following Java conventions for accessor methods.
Line range hint
1-121: Overall, the changes enhance theSubscribeServiceRequestclass effectively.The additions of new fields (
vipOnly,localCloudFirst,group, andproperties) and their corresponding getter/setter methods improve the functionality of the class. The changes are well-implemented and follow Java conventions.Consider the following suggestions for further improvement:
- Add Javadoc comments for the new fields to enhance documentation.
- Implement defensive copying for the
propertiesfield to prevent external state modification.- Include the
propertiesfield in thetoString()method for a complete string representation.These minor enhancements will improve the overall quality, maintainability, and robustness of the class.
registry/registry-mesh/src/test/java/com/alipay/sofa/rpc/registry/mesh/MeshRegistryTest.java (6)
188-189: LGTM! Appropriate application of the group parameter.The addition of
setParameters(parameter)to the ProviderConfig is consistent with the PR objective and correctly applies the group information.
202-203: LGTM! Consistent application of the group parameter.The addition of
setParameters(parameter)to the ConsumerConfig is consistent with the PR objective and maintains consistency with the ProviderConfig setup.
212-212: LGTM! Proper verification of group in PublishServiceRequest.The added assertion correctly verifies that the group information ("SOFA_TEST") is set in the PublishServiceRequest, ensuring the custom group support is working as expected.
236-236: LGTM! Verification of group in UnPublishServiceRequest.The added assertion correctly verifies that the group information ("SOFA_TEST") is set in the UnPublishServiceRequest, ensuring the custom group support is maintained during service unpublishing.
257-258: LGTM! Consistent application of group parameter for multiple consumers.The addition of
setParameters(parameter)to the second ConsumerConfig maintains consistency in applying the group information across multiple consumers. This ensures that the custom group support is properly tested for multiple subscriptions.
Line range hint
1-337: Overall assessment: Well-implemented custom group supportThe changes in this file consistently implement and verify the custom group support across various operations (publish, subscribe, unpublish, unsubscribe) in the MeshRegistry. The modifications align well with the PR objective of adding support for custom groups in the sofa registry.
Key points:
- Consistent application of group parameters to both provider and consumer configurations.
- Thorough assertions to verify group information in various service requests.
- Maintenance of group information across multiple consumers and operations.
Suggestions for further improvement:
- Consider using constants for repeated string values like "SOFA_TEST".
- Add more specific assertions for properties in SubscribeServiceRequest.
- Extend group assertions to batch operations for complete coverage.
These changes provide a solid foundation for custom group support in the Mesh Registry.
registry/registry-mesh/src/main/java/com/alipay/sofa/rpc/registry/mesh/MeshRegistry.java (3)
23-23: LGTM: Import added for StringUtilsThe addition of the
StringUtilsimport is appropriate for the new string operations in the modified methods.
241-244: LGTM: Group support added to UnPublishServiceRequestThe changes correctly implement group support for the unpublish service request, consistent with the
buildPublishServiceRequestmethod.Please verify if using
getStaticAttr()instead ofgetStaticAttrs()is intentional. If it's a typo, consider changing it togetStaticAttrs()for consistency.
315-319: LGTM: Group support added to SubscribeServiceRequestThe changes correctly implement group support for the subscribe service request.
Please verify:
- If setting
propertiesfromconsumerConfig.getParameters()is intentional and required for the subscribe request.- If using
consumerConfig.getParameter()for getting the group (instead ofgetStaticAttrs()orgetStaticAttr()as in provider methods) is the correct approach for consumer configs.
Mesh Registry support custom group for sofa registry.
Summary by CodeRabbit
New Features
Bug Fixes
Tests