Skip to content
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

Flip runtime guard on use_fast_protobuf_hash #33121

Merged
merged 4 commits into from
Apr 1, 2024

Conversation

ravenblackx
Copy link
Contributor

@ravenblackx ravenblackx commented Mar 26, 2024

Commit Message: Flip runtime guard on use_fast_protobuf_hash
Additional Description: #31276
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Raven Black <ravenblack@dropbox.com>
Copy link

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #33121 was opened by ravenblackx.

see: more, trace.

Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
@@ -108,6 +108,7 @@ RUNTIME_GUARD(envoy_restart_features_allow_client_socket_creation_failure);
RUNTIME_GUARD(envoy_restart_features_quic_handle_certs_with_shared_tls_code);
RUNTIME_GUARD(envoy_restart_features_send_goaway_for_premature_rst_streams);
RUNTIME_GUARD(envoy_restart_features_udp_read_normalize_addresses);
RUNTIME_GUARD(envoy_restart_features_use_fast_protobuf_hash);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there is no UT to check default values, is that common?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are unit tests that test both true and false behaviors for this case - I made the unit tests force the values specifically so that changing the default wouldn't require updating the tests.
It may not be common, but I find it much less annoying than the other way.

@soulxu
Copy link
Member

soulxu commented Apr 1, 2024

gentle ping @jmarantz

jmarantz
jmarantz previously approved these changes Apr 1, 2024
@jmarantz
Copy link
Contributor

jmarantz commented Apr 1, 2024

@yanavlasov for senior review.

Also this seems possibly related to the equal/equivalent change in another PR so I wanted to make sure you were aware of the change this enables, which provides an efficient way to get an order-insensitive hash for maps.

@jmarantz
Copy link
Contributor

jmarantz commented Apr 1, 2024

@ravenblackx needs main merge.

@yanavlasov
Copy link
Contributor

Waiting for main merge

/wait

Signed-off-by: Raven Black <ravenblack@dropbox.com>
@yanavlasov yanavlasov merged commit 690d3aa into envoyproxy:main Apr 1, 2024
53 checks passed
alyssawilk pushed a commit to alyssawilk/envoy that referenced this pull request Apr 29, 2024
---------

Signed-off-by: Raven Black <ravenblack@dropbox.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants