Skip to content
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

Improve EurekaConfigServerInstanceProvider #4267

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

Puppy4C
Copy link
Contributor

@Puppy4C Puppy4C commented Mar 9, 2024

The parameter regions in the client.getApplications() method represents remoteRegions, which indicates that the registry should not only retrieve and return a list of its own nodes, but also return a list of remoteRegions.

But config.getRegion() represents the local region, and if this parameter is included, the registry will continue to print:
No remote registry available for the remote region xxx

We should refer to com.netflix.discovery.DiscoveryClient#getAndStoreFullRegistry and use config.fetchRegistryForRemoteRegions() instead of it

Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Hello @Puppy4C, thanks for submitting the PR. Please fix the tests and make sure the build passes. Also, update the year in the license comment of any files you change to 2013-2024.

@Puppy4C Puppy4C changed the title Remove config.getRegion() when obtaining configserver Improve EurekaConfigServerInstanceProvider Mar 30, 2024
@Puppy4C
Copy link
Contributor Author

Puppy4C commented Mar 30, 2024

Hi @OlgaMaciaszek ,I have fixed the tests and updated the license comment,please review.

Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Thanks, @Puppy4C, please see new comments.

Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Thanks, @Puppy4C. LGTM.

@OlgaMaciaszek OlgaMaciaszek merged commit 47d3f71 into spring-cloud:main Apr 3, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants