Skip to content

feat: Add NacosServiceInstanceConverter util - #3686

Open
karsonto wants to merge 2 commits into
alibaba:2023.xfrom
karsonto:dev-converter
Open

feat: Add NacosServiceInstanceConverter util#3686
karsonto wants to merge 2 commits into
alibaba:2023.xfrom
karsonto:dev-converter

Conversation

@karsonto

@karsonto karsonto commented Apr 12, 2024

Copy link
Copy Markdown

some enhancement for nacos discovery

@CLAassistant

CLAassistant commented Apr 12, 2024

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@yuluo-yx

Copy link
Copy Markdown
Collaborator

Can you explain why it was modified in this way?

@karsonto

karsonto commented Jul 29, 2024

Copy link
Copy Markdown
Author

Can you explain why it was modified in this way?

This design can make the conversion between nacos Instance and spring cloud ServiceInstance clearly.

@oss-sentinel-ai oss-sentinel-ai left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

LGTM — clean refactoring that extracts Instance ↔ ServiceInstance conversion logic into a dedicated NacosServiceInstanceConverter utility class with a builder pattern. This eliminates duplicated conversion code in NacosBalancer and NacosServiceDiscovery.

Review Notes

  • Good use of builder pattern for bidirectional conversion
  • Private constructor prevents instantiation — correct for a utility class
  • Minor: The indentation in NacosBalancer changed from 1-tab to 2-tab. Consider aligning with the existing codebase style to minimize diff noise.

Automated review by github-manager-bot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants