Move Bytestream to array config #1951
Conversation
palfrey
force-pushed
the
bytestream-array-config
branch
from
dc07539 to
c2f1fad
Compare
palfrey
force-pushed
the
bytestream-array-config
branch
from
c2f1fad to
bed4c17
Compare
palfrey
force-pushed
the
bytestream-array-config
branch
from
bed4c17 to
4a49318
Compare
palfrey
force-pushed
the
bytestream-array-config
branch
from
4a49318 to
7085f1a
Compare
MarcusSorealheis
left a comment
MarcusSorealheis
left a comment
There was a problem hiding this comment.
First of all, this PR adds a lot more consistency with the other stores. Thank you for that.
MarcusSorealheis
left a comment
MarcusSorealheis
left a comment
There was a problem hiding this comment.
Overall I like this PR. All my comments are nits. One detail I will call out is that we might want to add duplicate name detection for instances. I won’t block this PR over that as we can add it easily eventually.
| #[serde(default, deserialize_with = "convert_data_size_with_shellexpand")] | ||
| pub max_bytes_per_stream: usize, | ||
|
|
||
| /// Maximum number of bytes to decode on each grpc stream chunk. |
There was a problem hiding this comment.
This is a breaking change in security boundaries. If someone had different message size limits for different services, they’ll lose that capability.
It’s a fringe use case so should be fine but we should check with the Chromium users.
There was a problem hiding this comment.
👍🏻 In theory we could do something where we support setting all the http_config stuff at a per-service level, but that would require redoing the config again. It does feel like this being supported for bytestream but not any of the other services is a bit odd TBH.
| @@ -0,0 +1,42 @@ | |||
| { | |||
| stores: [], | |||
There was a problem hiding this comment.
Should this be called legacy_bytestream_config.json5 or something?
There was a problem hiding this comment.
It's actually the legacy config for all the services, but I'll rename it as the legacy config
|
Tests should pass twice before merging |
palfrey
force-pushed
the
bytestream-array-config
branch
from
7085f1a to
ce7f767
Compare
palfrey
force-pushed
the
bytestream-array-config
branch
from
ce7f767 to
a93af2f
Compare
Both ce7f767fe1a094407ed366a5837e526130af44e2 and a93af2f (whose difference is a removal of the "always a particular value" generic from |
|
More than good enough :P |
2 participants
Description
Like #1806, but with backwards compatibility. Note, that when upgrading the config the
max_decoding_message_sizehas moved into thehttppart of the config as it's set per-server.Type of change
Please delete options that aren't relevant.
How Has This Been Tested?
bazel test //...Checklist
bazel test //...passes locallygit amendsee some docsThis change is