KEP-5116: Add KEP (Streaming response encoding)#5119
Conversation
|
@benluddy can you ptal? |
05be44a to
7267cf4
Compare
|
/assign @jpbetz |
032c3d0 to
8b5656f
Compare
2c8682a to
80a5cb2
Compare
sftim
left a comment
There was a problem hiding this comment.
Thanks. Here's a bunch of feedback.
Key points:
- let's be clear about the status of YAML and that streaming encoding to YAML won't be available
- (does using legacy encoding to YAML provide a route for a malicious client to overload API server memory)?
- formally, the response to a list is a collection
- we style list in lower case bold in docs, and I recommend doing that here too.
|
BTW if we want to ship JSON and ProtoBuf as beta for v1.33, I don't see anything stopping us doing that and still adding streaming YAML collections before we close the KEP as Done. |
| Then, we will rewrite the gzip compression to buffer the response and delay the | ||
| decision to enable compression until we have observed enough bytes to hit the threshold | ||
| or we received whole response and we can write it without compressing. |
There was a problem hiding this comment.
just an implementation note, by adding an intermediate buffer to the gzip writer wrapper, be careful not to reintroduce the issue where we don't flush headers until a non-zero number of bytes are written
maybe the code path that hits deferredResponseWriter isn't vulnerable to that issue if it already has the single object it is serializing, but watch out for buffering a generic gzip writer wrapper if we're not sure whether or not we'll have immediate output
|
a couple suggestions but lgtm overall |
dfa1854 to
fbcb030
Compare
|
/lgtm |
Co-authored-by: Tim Bannister <tim@scalefactory.com>
|
/retitle KEP-5116: Add KEP (Streaming response encoding) |
|
LGTM. Strictly constraining the supported types keeps the special-case implementation simple, and client behavior remains unchanged. |
|
needs updates for PRR enable/disable and monitoring. The list of tests looks good. lgtm otherwise. |
Fixed, thanks for review! |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, fuweid, serathius The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
#5116
/assign @deads2k @liggitt