-
Notifications
You must be signed in to change notification settings - Fork 3.9k
A68 random subsetting #12377
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
base: master
Are you sure you want to change the base?
A68 random subsetting #12377
Conversation
9178308
to
aefee00
Compare
aefee00
to
1870e6f
Compare
util/src/main/java/io/grpc/util/RandomSubsettingLoadBalancer.java
Outdated
Show resolved
Hide resolved
util/src/main/java/io/grpc/util/RandomSubsettingLoadBalancer.java
Outdated
Show resolved
Hide resolved
util/src/main/java/io/grpc/util/RandomSubsettingLoadBalancer.java
Outdated
Show resolved
Hide resolved
util/src/main/java/io/grpc/util/RandomSubsettingLoadBalancer.java
Outdated
Show resolved
Hide resolved
This reverts commit 8d349df.
@ejona86 - PR updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sending what I have. I expect I'll have a few more comments, but I suspect nothing that impacts the changes for these comments.
util/src/main/java/io/grpc/util/RandomSubsettingLoadBalancer.java
Outdated
Show resolved
Hide resolved
util/src/main/java/io/grpc/util/RandomSubsettingLoadBalancer.java
Outdated
Show resolved
Hide resolved
util/src/test/java/io/grpc/util/RandomSubsettingLoadBalancerTest.java
Outdated
Show resolved
Hide resolved
util/src/main/java/io/grpc/util/RandomSubsettingLoadBalancerProvider.java
Show resolved
Hide resolved
util/src/main/java/io/grpc/util/RandomSubsettingLoadBalancer.java
Outdated
Show resolved
Hide resolved
util/src/main/java/io/grpc/util/RandomSubsettingLoadBalancer.java
Outdated
Show resolved
Hide resolved
@ejona86 - PR updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to stare at BackendDetails a bit to see what's going on there with the Answer, but sending what I have.
util/src/main/java/io/grpc/util/RandomSubsettingLoadBalancer.java
Outdated
Show resolved
Hide resolved
Long subsetSize = JsonUtil.getNumberAsLong(rawConfig, "subsetSize"); | ||
if (subsetSize == null) { | ||
return ConfigOrError.fromError( | ||
Status.INTERNAL.withDescription( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should probably be UNAVAILABLE. INTERNAL isn't appropriate, because nothing in gRPC was busted; this was just wrong configuration. I suspect you probably copied this from OutlierDetectionLoadBalancerProvider
, which itself is wrong. There's really no difference here than on line 57 if we get a RuntimeException. #12409 will fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was looking at other providers code and as well old java solution to understand which Status I am supposed to use.
Does the same apply to child policy parsing? I can see that GracefulSwitchLoadBalancer.parseLoadBalancingPolicyConfig(...)
is using INTERNAL to represent parsing failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the file to
grpc-java/xds/third_party/envoy/import.sh
Lines 26 to 30 in 349a35a
FILES=( | |
envoy/admin/v3/config_dump.proto | |
envoy/admin/v3/config_dump_shared.proto | |
envoy/annotations/deprecation.proto | |
envoy/config/accesslog/v3/accesslog.proto |
And re-run the script to download the same version as the rest of the Envoy commit we are using.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it seems like there is nothing like random_subsetting
LB policy in envoy. The closest seems random
or subset
, but those aren't what we exactly need.
How should I tackle this problem? Should I update the envoy API before merging this PR then?
@ejona86 - pushed changes. However, I do have two questions, which I have posted as answers to your comments. |
implementing gRFC A65 grpc/proposal/pull/423
This change contains:
Relocation ofUsage ofXxHash64
library, so it can be shared betweenutil
andxds
projects. Proposed source directory is:third-party/zero-allocation-hashing
.murmur3_128
hashing algorithm from Guava library.RandomSubsettingLoadBalancer
andRandomSubsettingLoadBalancerProvider
classes and integration into theutil
project.random_subsetting
LB policy and as well new envoy proto message.