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.

Sign up for GitHub

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

Jump to bottom

feat(lb): deterministic subsetting algorithm #1289

Merged
merged 12 commits into from
Nov 13, 2023
Merged

feat(lb): deterministic subsetting algorithm #1289

merged 12 commits into from
Nov 13, 2023

Conversation

jizhuozhi
Copy link
Contributor

@jizhuozhi jizhuozhi commented Oct 10, 2023 •
edited
Loading

@jizhuozhi
Copy link
Contributor Author

Hello @OlgaMaciaszek , PTAL

@spencergibb

This comment was marked as resolved.

Sign in to view
@jizhuozhi

This comment was marked as resolved.

Sign in to view
@OlgaMaciaszek OlgaMaciaszek self-assigned this Oct 25, 2023
@OlgaMaciaszek OlgaMaciaszek added this to the 4.1.0-RC1 milestone Oct 25, 2023
OlgaMaciaszek
OlgaMaciaszek requested changes Oct 30, 2023
View reviewed changes
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 @jizhuozhi, thanks for submitting the PR. I have done an initial review after looking at the class structure. I have added some comments - please address. Also, please add documentation. I will take a look at the actual algorithm implementation and tests tomorrow and provide a second review then.

...main/java/org/springframework/cloud/loadbalancer/core/SubsetServiceInstanceListSupplier.java Outdated Show resolved Hide resolved
...main/java/org/springframework/cloud/loadbalancer/core/SubsetServiceInstanceListSupplier.java Outdated Show resolved Hide resolved
OlgaMaciaszek
OlgaMaciaszek requested changes Oct 31, 2023
View reviewed changes
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.

Have added some comments. Please address. Please also add corresponding ServiceInstanceListSupplierBuilder methods and configurations property-based instantiation in LoadBalancerClientConfiguration.

...main/java/org/springframework/cloud/loadbalancer/core/SubsetServiceInstanceListSupplier.java Show resolved Hide resolved
...main/java/org/springframework/cloud/loadbalancer/core/SubsetServiceInstanceListSupplier.java Show resolved Hide resolved
.../java/org/springframework/cloud/loadbalancer/core/SubsetServiceInstanceListSupplierTest.java Show resolved Hide resolved
.../java/org/springframework/cloud/loadbalancer/core/SubsetServiceInstanceListSupplierTest.java Outdated Show resolved Hide resolved
.../java/org/springframework/cloud/loadbalancer/core/SubsetServiceInstanceListSupplierTest.java Show resolved Hide resolved
OlgaMaciaszek
OlgaMaciaszek requested changes Nov 2, 2023
View reviewed changes
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.

Have added some comments - please address them.

...mons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerProperties.java Outdated Show resolved Hide resolved
...mons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerProperties.java Show resolved Hide resolved
OlgaMaciaszek
OlgaMaciaszek reviewed Nov 2, 2023
View reviewed changes
...main/java/org/springframework/cloud/loadbalancer/core/SubsetServiceInstanceListSupplier.java Outdated
super(delegate);
LoadBalancerProperties properties = factory.getProperties(getServiceId());
String instanceId = properties.getSubset().getInstanceId();
if (StringUtils.hasText(instanceId)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please document this entire logic.

@OlgaMaciaszek OlgaMaciaszek modified the milestones: 4.1.0-RC1, 4.1.0 Nov 2, 2023
@OlgaMaciaszek
Copy link
Collaborator

Hi @jizhuozhi, will you be able to finish this PR up soon? I'd like to get this feature included in 4.1.0.

OlgaMaciaszek
OlgaMaciaszek requested changes Nov 9, 2023
View reviewed changes
Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Have added a comment. Please address.

.../java/org/springframework/cloud/loadbalancer/annotation/LoadBalancerClientConfiguration.java Show resolved Hide resolved
OlgaMaciaszek
OlgaMaciaszek requested changes Nov 9, 2023
View reviewed changes
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 @jizhuozhi, now only documentation, including this: #1289 (comment), is left. Please add corresponding changes in docs/modules/ROOT/pages/spring-cloud-commons/loadbalancer.adoc.

@jizhuozhi
Copy link
Contributor Author

Thanks @jizhuozhi, now only documentation, including this: #1289 (comment), is left. Please add corresponding changes in docs/modules/ROOT/pages/spring-cloud-commons/loadbalancer.adoc.

Hello, @OlgaMaciaszek . The document has been updated, PTAL

OlgaMaciaszek
OlgaMaciaszek approved these changes Nov 10, 2023
View reviewed changes
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 for working on it, @jizhuozhi. LGTM.

@OlgaMaciaszek
Copy link
Collaborator

Hi @Buzzardo, could you please review documentation changes?

@OlgaMaciaszek
Copy link
Collaborator

Fixes gh-1288.

@OlgaMaciaszek OlgaMaciaszek linked an issue Nov 10, 2023 that may be closed by this pull request
deterministic subsetting algorithm to limit the number of instances #1288
Closed
@OlgaMaciaszek OlgaMaciaszek merged commit cefe2b5 into spring-cloud:main Nov 13, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Buzzardo Buzzardo Buzzardo approved these changes

@OlgaMaciaszek OlgaMaciaszek OlgaMaciaszek approved these changes

Assignees

@OlgaMaciaszek OlgaMaciaszek

Labels
enhancement
Projects
No open projects
2023.0.0
Status: Done
Milestone
4.1.0
Development

Successfully merging this pull request may close these issues.

deterministic subsetting algorithm to limit the number of instances
5 participants
@jizhuozhi @spencergibb @OlgaMaciaszek @Buzzardo @spring-cloud-issues