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

Deterministic Subsetting Lb Policy #10470

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

joybestourous
Copy link
Contributor

@joybestourous joybestourous commented Aug 10, 2023

A POC for grpc/proposal#383

@joybestourous joybestourous changed the title subsetting java implementation Deterministic Subsetting Lb Policy Aug 15, 2023
@joybestourous joybestourous marked this pull request as ready for review August 18, 2023 21:08
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

(For the record, I had been trying to get to reviewing this the last two weeks.)

This isn't a detailed review, mostly looking at the shape of things. But it looks pretty good. There will obviously be changes to adjust to the final A68.

* https://github.com/grpc/proposal/blob/master/A68-deterministic-subsetting-lb-policy.md
*/
@Internal
public final class DeterministicSubsettingLoadBalancer extends LoadBalancer {
Copy link
Member

Choose a reason for hiding this comment

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

Remove the public (and thus @Internal). Only the Provider needs to be public.

(DeterministicSubsettingLoadBalancerConfig)
resolvedAddresses.getLoadBalancingPolicyConfig();

switchLb.switchTo(config.childPolicy.getProvider());
Copy link
Member

Choose a reason for hiding this comment

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

Since you made this change, I've cleaned this up and switchTo() is now deprecated. Instead of switchTo, you'll just use setLoadBalancingPolicyConfig() and pass a GracefulSwitchLB configuration object. This should clean up the parsing in the logic in the provider, as now GracefulSwitchLB.parseLoadBalancingPolicyConfig() does much of the work instead of random internal APIs.

2c49cc4 might be a reasonable example of the change.


@VisibleForTesting
static class AddressComparator implements Comparator<SocketAddress> {
// For consistency with the golang instrumentation, this assumes toString is overridden such
Copy link
Member

Choose a reason for hiding this comment

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

This isn't consistent with golang. I know this will change to hashing, but there will still be some similar issues. Maybe we'll be forced to use address.hashCode(), or maybe it doesn't matter because we just have to be consistent within a single implementation. See f866c80 where I mention some issues with the string representation.

@larry-safran
Copy link
Contributor

@joybestourous Are you still interested in getting this PR merged?

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.

3 participants