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

feat(apollo-client): the spi of config service load balancer client #4394

Merged

Conversation

Anilople
Copy link
Contributor

@Anilople Anilople commented Jun 3, 2022

What's the purpose of this PR

Let user can custom load balancer client of how to choose config service by spi.

Which issue(s) this PR fixes:

Fixes #3225

Brief changelog

  • interface ConfigServiceLoadBalancerClient
  • implementation RandomConfigServiceLoadBalancerClient
  • implementation DefaultConfigServiceLoadBalancerClient extends RandomConfigServiceLoadBalancerClient
  • use java.util.concurrent.ThreadLocalRandom instead of java.util.Random

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Read the Contributing Guide before making this pull request.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit tests to verify the code.
  • Run mvn clean test to make sure this pull request doesn't break anything.
  • Update the CHANGES log.

@Anilople Anilople added feature Categorizes issue as related to a new feature. area/client apollo-client labels Jun 3, 2022
@Anilople
Copy link
Contributor Author

Anilople commented Jun 3, 2022

todo:

  • changelog
  • document

@Anilople
Copy link
Contributor Author

Anilople commented Jun 3, 2022

Should update the version of project before this pr merge?

apollo/pom.xml

Line 63 in 40772c4

<revision>2.0.1</revision>

@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2022

Codecov Report

Merging #4394 (6ed0772) into master (2cc9fc8) will increase coverage by 0.00%.
The diff coverage is 92.30%.

@@            Coverage Diff            @@
##             master    #4394   +/-   ##
=========================================
  Coverage     53.20%   53.20%           
- Complexity     2677     2681    +4     
=========================================
  Files           489      490    +1     
  Lines         15291    15300    +9     
  Branches       1585     1586    +1     
=========================================
+ Hits           8135     8141    +6     
- Misses         6602     6604    +2     
- Partials        554      555    +1     
Impacted Files Coverage Δ
...llo/spi/RandomConfigServiceLoadBalancerClient.java 87.50% <87.50%> (ø)
.../apollo/internals/RemoteConfigLongPollService.java 78.44% <100.00%> (+0.12%) ⬆️
...rk/apollo/spring/property/SpringValueRegistry.java 83.78% <0.00%> (-5.41%) ⬇️
.../framework/apollo/spring/property/SpringValue.java 87.71% <0.00%> (-1.76%) ⬇️
...ervice/service/ReleaseMessageServiceWithCache.java 87.05% <0.00%> (+1.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cc9fc8...6ed0772. Read the comment docs.

@nobodyiam nobodyiam added this to the 2.1.0 milestone Jun 4, 2022
@nobodyiam
Copy link
Member

Should update the version of project before this pr merge?

apollo/pom.xml

Line 63 in 40772c4

<revision>2.0.1</revision>

Right, we could merge this pr once 2.0.1 is released.

zouyx
zouyx previously approved these changes Jun 8, 2022
Copy link
Member

@zouyx zouyx left a comment

Choose a reason for hiding this comment

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

LGTM

@nobodyiam
Copy link
Member

As 2.1.0 is released, please rebase the code and also update the CHANGES.md.

@Anilople Anilople marked this pull request as ready for review June 11, 2022 02:40
@Anilople Anilople requested review from zouyx and a team June 11, 2022 02:40
@Anilople Anilople requested a review from nobodyiam June 12, 2022 01:26
Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

BTW, is it necessary to also update the lb logic in RemoteConfigRepository?

docs/en/usage/java-sdk-user-guide.md Outdated Show resolved Hide resolved
docs/en/usage/java-sdk-user-guide.md Outdated Show resolved Hide resolved
docs/en/usage/java-sdk-user-guide.md Outdated Show resolved Hide resolved
@Anilople
Copy link
Contributor Author

BTW, is it necessary to also update the lb logic in RemoteConfigRepository?

According the code

for (int i = 0; i < maxRetries; i++) {
List<ServiceDTO> randomConfigServices = Lists.newLinkedList(configServices);
Collections.shuffle(randomConfigServices);
//Access the server which notifies the client first
if (m_longPollServiceDto.get() != null) {
randomConfigServices.add(0, m_longPollServiceDto.getAndSet(null));
}
for (ServiceDTO configService : randomConfigServices) {
if (onErrorSleepTime > 0) {

all ConfigServices will be accessed.

if we use random strategy to instead of it, maybe lose the correct one ConfigService?

@nobodyiam
Copy link
Member

According the code
all ConfigServices will be accessed.

@Anilople Actually it's a random strategy to access the config service until some config service returns the config. So it won't access all config services unless every attempts to get the config fails. However, I think it may not be that necessary to customize its logic as it is only triggered when the config changes. So it is OK now.

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

LGTM

@nobodyiam nobodyiam merged commit 1c4b38e into apolloconfig:master Jun 13, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jun 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/client apollo-client feature Categorizes issue as related to a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

client 访问config server负载均衡算法
4 participants