Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
route match: Add runtime_fraction field for more granular routing #4217
route match: Add runtime_fraction field for more granular routing #4217
Changes from 5 commits
9490c37
9f7fc09
dad5be2
7ee808d
7e92dca
e40132c
fca465f
6d5c6bc
1ff4323
4df9106
336122e
4121f9d
aee0a34
87bc01a
cb67993
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should this be in
api/envoy/api/v2/core/base.proto
so as to be next toRuntimeUint32
?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 didn't do that because I didn't want to introduce a dependency on percent.proto. It was originally in percent.proto, but an earlier comment from @danielhochman suggested I move it out into its own file.
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.
It's fine for base to depend on percent.proto (just not the other way around).
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.
Nice to see this approach. I actually think we should use the JSON/YAML encoding on second thoughts, since text proto is not a standard. If you go this route, we have some helpers to make it easy to parse, see https://github.com/envoyproxy/envoy/blob/master/source/common/protobuf/utility.h#L210. This will also validate the proto for you for constraints, which is missing in the parsing below.
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.
If I read this right, failure to convert the denominator key into a valid denominator value causes us to use the numerator key's value with the default value's denominator. That seems like it might cause some pretty unexpected behavior. Is it reasonable to just switch using the default value altogether in this case?
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 think what you propose is the sane thing to do. I'll change the logic.
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.
Actually- scratch that. A user would have unexpected behavior either way if they misspell a denominator type and it switches to a default value. I'm not sure one way is better than the other. @htuch what do you think?
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.
Not sure what the best practice to alert when runtime is wrong; how do folks operationally determine when a runtime change has taken effect? Via
/runtime
?One thought that occurred to me just now is that instead of having separate values for numerator/denominator, we have a single values, which is a textual representation of a
FractionalPercent
proto. That way, we can just use PGV validation on this to check sanity. It also solves an update atomicity issue.