Skip to content

feat(lb): deterministic subsetting algorithm#1289

Merged
OlgaMaciaszek merged 12 commits intospring-cloud:mainfrom
jizhuozhi:main
Nov 13, 2023
Merged

feat(lb): deterministic subsetting algorithm#1289
OlgaMaciaszek merged 12 commits intospring-cloud:mainfrom
jizhuozhi:main

Conversation

@jizhuozhi
Copy link
Copy Markdown
Contributor

@jizhuozhi jizhuozhi commented Oct 10, 2023 •
edited
Loading

@jizhuozhi
Copy link
Copy Markdown
Contributor Author

jizhuozhi commented Oct 11, 2023

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
Copy Markdown
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.

Comment thread ...main/java/org/springframework/cloud/loadbalancer/core/SubsetServiceInstanceListSupplier.java Outdated
Comment thread ...main/java/org/springframework/cloud/loadbalancer/core/SubsetServiceInstanceListSupplier.java Outdated
OlgaMaciaszek
OlgaMaciaszek requested changes Oct 31, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Comment thread ...main/java/org/springframework/cloud/loadbalancer/core/SubsetServiceInstanceListSupplier.java
Comment thread ...main/java/org/springframework/cloud/loadbalancer/core/SubsetServiceInstanceListSupplier.java
Comment thread .../java/org/springframework/cloud/loadbalancer/core/SubsetServiceInstanceListSupplierTest.java
Comment thread .../java/org/springframework/cloud/loadbalancer/core/SubsetServiceInstanceListSupplierTest.java Outdated
Comment thread .../java/org/springframework/cloud/loadbalancer/core/SubsetServiceInstanceListSupplierTest.java
OlgaMaciaszek
OlgaMaciaszek requested changes Nov 2, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Comment thread ...mons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerProperties.java Outdated
Comment thread ...mons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerProperties.java
OlgaMaciaszek
OlgaMaciaszek reviewed Nov 2, 2023
View reviewed changes
Comment thread ...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
Copy Markdown
Collaborator

@OlgaMaciaszek OlgaMaciaszek Nov 2, 2023

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
Copy Markdown
Collaborator

OlgaMaciaszek commented Nov 9, 2023

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
Copy Markdown
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.

Comment thread .../java/org/springframework/cloud/loadbalancer/annotation/LoadBalancerClientConfiguration.java
OlgaMaciaszek
OlgaMaciaszek requested changes Nov 9, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

jizhuozhi commented Nov 9, 2023

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
Copy Markdown
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
Copy Markdown
Collaborator

OlgaMaciaszek commented Nov 10, 2023

Hi @Buzzardo, could you please review documentation changes?

@OlgaMaciaszek
Copy link
Copy Markdown
Collaborator

OlgaMaciaszek commented Nov 10, 2023

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
Buzzardo
Buzzardo suggested changes Nov 13, 2023
View reviewed changes
Comment thread docs/modules/ROOT/pages/spring-cloud-commons/loadbalancer.adoc Outdated
Comment thread docs/modules/ROOT/pages/spring-cloud-commons/loadbalancer.adoc Outdated
@OlgaMaciaszek OlgaMaciaszek merged commit cefe2b5 into spring-cloud:main Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@OlgaMaciaszek OlgaMaciaszek OlgaMaciaszek approved these changes

+1 more reviewer

@Buzzardo Buzzardo Buzzardo approved these changes

Reviewers whose approvals may not affect merge requirements

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