ringhash: allow overriding max ringhash size via environment variable #5884
Conversation
internal/envconfig/xds.go
Outdated
|
|
||
| // XDSRingHashLimit indicates the maximum ring size which defaults to 4096 but | ||
| // may be overridden by setting the environment variable | ||
| // "GRPC_XDS_RING_HASH_LIMIT". A hard maximum limit of 8MB and minimum limit |
There was a problem hiding this comment.
RING_HASH_LIMIT is somewhat ambiguous, since this limit really controls the max number of ring hash entries, and only indirectly controls memory (e.g. per-entry memory consumption could be reduced with a revised layout).
Note too the A hard maximum limit of 8MB should say something like A hard maximum limit of 8 million entries, which can consume ~256MB.
There was a problem hiding this comment.
RING_HASH_LIMIT
Can you suggest something better? I wanted to avoid something too verbose. GRPC_XDS_RING_SIZE_LIMIT is probably too vague, and GRPC_XDS_RING_HASH_RING_SIZE_LIMIT is getting pretty long. Is GRPC_XDS_RING_HASH_SIZE_LIMIT still too ambiguous?
Note too the A hard maximum limit of 8MB should say something like A hard maximum limit of 8 million entries, which can consume ~256MB
Well, it's apparently not 8 million either, it's 8,388,608. Why did we choose that number (for Java) btw?
There was a problem hiding this comment.
Oh wait.. we actually NACK for configs > 8M. So probably this is irrelevant to even mention here?
There was a problem hiding this comment.
In C++, their's a channel option called GRPC_ARG_RING_HASH_LB_RING_SIZE_CAP.
In Java, the global is setRingSizeCap.
So maybe use GRPC_XDS_RING_HASH_CAP ?
Oh wait.. we actually NACK for configs > 8M. So probably this is irrelevant to even mention here?
Yeah I think it's not really relevant. Though I suppose we could mention that GRPC_XDS_RING_HASH_CAP does not influence the validation of max ring size configuration.
There was a problem hiding this comment.
So maybe use
GRPC_XDS_RING_HASH_CAP?
This is fine since it matches the other languages re: cap/limit, though I find "cap" to be less formal/precise than "limit".
Changed.
There was a problem hiding this comment.
Actually, should we remove "xds" from the name, too, since the LB policy name doesn't have "xds" in it?
There was a problem hiding this comment.
(I went ahead and did that in another commit; LMK.)
internal/envconfig/envconfig.go
Outdated
| @@ -36,4 +37,23 @@ var ( | |||
| // AdvertiseCompressors is set if registered compressor should be advertised | |||
| // ("GRPC_GO_ADVERTISE_COMPRESSORS" is not "false"). | |||
| AdvertiseCompressors = !strings.EqualFold(os.Getenv(advertiseCompressorsStr), "false") | |||
| // XDSRingHashCap indicates the maximum ring size which defaults to 4096 | |||
There was a problem hiding this comment.
nit: s/XDSRingHashCap/RingHashCap
There was a problem hiding this comment.
Not just a nit; vet won't pass like this. Fixed.
internal/envconfig/envconfig_test.go
Outdated
| want uint64 | ||
| }{ | ||
| { | ||
| name: "error parsing; want default", |
There was a problem hiding this comment.
Should we change the test names to be in snake_case instead, as the go test command would do that anyways, and will ;s in there, the test name might look weird.
There was a problem hiding this comment.
I think this is fine IMO. AFAICT there's no real guidance given on this by the Go team, and the name is essentially just a convenience & informational. No matter what we name things, they will be copy-pasteable to re-run. That said, the "want"s are pretty obvious, especially given the "want" declaration, so I just removed them.
| if tc.val == "" { | ||
| os.Unsetenv(testVar) |
There was a problem hiding this comment.
Do we really need this special case for empty string? Since the code which reads the env var uses os.GetEnv(), and does not distinguish between whether the env var is set to empty string vs being unset, can't we simply set it to the empty string here (for the appropriate test case)?
There was a problem hiding this comment.
From a blackbox testing perspective, it's an interesting, specific test. But from a whitebox perspective, it's no different. So I could go either way, but I'm inclined to keep it with the former in mind.
Implement the remaining part of grpc/proposal#338 after #5801 (allowing the maximum size to be overridden).
RELEASE NOTES:
GRPC_RING_HASH_CAPenvironment variable to override the maximum ring size.