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
Reflection-based deterministic message hashing #30761
Reflection-based deterministic message hashing #30761
Changes from 38 commits
9c55848
e9ca067
44f7ebe
465d833
0bfb6f1
ef99f1e
11aba21
a5b4a89
6c7d090
29532f6
aa9e5fd
9e38e75
edcf363
9b67119
c172e2d
c686b3a
5b4e623
cd2d3c1
38c7f6d
b6e84b0
3320ed0
5afe859
da47756
e128ee4
974f4f0
5b0b818
f5393b6
8830f12
f308278
2104c6e
234cbc8
7b737bc
c829856
ecdd501
2757c86
3baddeb
40cefb2
fffa18c
4255b62
23c6aa9
7feee62
f6a44ef
4455b88
fbed541
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.
how would you feel about making this default to the current behavior.
I would like, when we import this into our mono-repo, to have products run the new mode in staging for a few weeks first before we turn it on.
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 would prefer not to. My understanding is that runtime guards for performance improvements are generally supposed to be emergency shutoffs, not optional activates - if you make it opt-in then it doesn't get tested.
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 forever. Just for some amount of time (a quarter?)
@alyssawilk may have more context on an appropriate amount of time a large change should bake before flipping it to the default. It will improve things; it will get tested and turned on.
I just don't want the eventual productionizing of this to catch us by surprise, and we wouldn't be able to disable it prior to importing it.
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.
False by default it is.
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.
generally when we do false by default we assign an owner to hassle when we flip true. Josh as you're looking at testing this are you OK driving the default flip? Alternately if this is no higher risk than most flags should we leave it as true by default upstream and import-false? I'm fine either way, just want to make sure we have a plan on how to move forward
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 I don't need to take that on -- I like this change a lot, but it's solving a problem our service is not currently suffering from, afaict. Even if we have like a delay till end of January to make sure we have a chance to flip the flag off, I'd be OK.
I think for the service my team runs, we are being particularly paranoid right now, and we'd just want a chance to have this flag imported into our monorepo first, and then turn it off, then it could be turned on in Envoy main. After N months we could flip it on ourselves; we just can't take on that risk right now.