Change default values for two GRPC setting we have to set so the queriers can connect to a frontend or scheduler #4435
Conversation
trevorwhitney
force-pushed
the
better-grpc-defaults
branch
from
469c627 to
658a06e
Compare
| throwaway := flag.NewFlagSet("throwaway", flag.PanicOnError) | ||
|
|
||
| // Register to throwaway flags first. Default values are remembered during registration and cannot be changed, | ||
| // but we can take values from throwaway flag set and reregister into supplied flags with new default values. |
There was a problem hiding this comment.
Q: how is it different than just changing default values of those two configs directly in c.Server.RegisterFlags?
It's confusing because, all our default values are in c.Server.RegisterFlags() currently. And IMHO, its bit odd to move only those two into separate function with throwaway flagset :)
I see you tried explaining it in the comment. But I think I failed to understand what that exactly means and why it matters here :( Shouldn't this be treated as normal default values?
There was a problem hiding this comment.
@kavirajk c.Server.RegisterFlags() is defined in the weaveworks/common library. We could make a PR into that if we think this is a better default for all cases, but I'm not sure if that's the case. I think we need to change this because of something to do specifically with memberlist. They're currently using the grpc defaults which they might prefer to keep. Either way, I still think this makes sense in the meantime to override them here until such a PR is made and merged.
There was a problem hiding this comment.
I also wish there were a better way.
cyriltovena
left a comment
cyriltovena
left a comment
There was a problem hiding this comment.
LGTM Let's wait for @KMiller-Grafana
Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>
Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>
trevorwhitney
force-pushed
the
better-grpc-defaults
branch
from
658a06e to
63663a2
Compare
KMiller-Grafana
left a comment
KMiller-Grafana
left a comment
There was a problem hiding this comment.
Docs look good to me.
4 participants
What this PR does / why we need it:
This changes two default configuration values:
Which issue(s) this PR fixes:
Fixes #4434
Special notes for your reviewer:
Checklist