Streaming json list encoder#129334
Conversation
038e705 to
1b4e17e
Compare
|
💭 If we wanted to do streaming encoding to YAML or CBOR, is there anything in this code making the YAML / CBOR option hard to do? |
|
Changelog suggestion -List responses in JSON are marshalled one element at the time, drastically reducing memory needed to serve large responses. Can be disabled via StreamingJSONListEncoding feature flag
+Improved how the API server responds to **list** requests where the response format negotiates to JSON. List responses in JSON are marshalled one element at the time, drastically reducing memory needed to serve large collections. Streaming list responses can be disabled via the `StreamingJSONListEncoding` feature gate.(will need adjusting if the feature gate name changes per my opinion / suggestion) |
Don't know, my focus is on JSON for CRDs and than Proto for built in types. I would need to get some help for the others. |
42ab20e to
6148aef
Compare
6148aef to
92837da
Compare
92837da to
85425d6
Compare
Used test cases from: * Original PR kubernetes#129334 * KEP https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/5116-streaming-response-encoding#unit-tests For now testing current serializer implementation to show encoder behavior and agree on set of tests. Having a separate PR should make review easier. In separate PR will add the implementation for streaming that should provide same response byte-to-byte.
Used test cases from: * Original PR kubernetes#129334 * KEP https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/5116-streaming-response-encoding#unit-tests For now testing current serializer implementation to show encoder behavior and agree on set of tests. Having a separate PR should make review easier. In separate PR will add the implementation for streaming that should provide same response byte-to-byte.
Used test cases from: * Original PR kubernetes#129334 * KEP https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/5116-streaming-response-encoding#unit-tests For now testing current serializer implementation to show encoder behavior and agree on set of tests. Having a separate PR should make review easier. In separate PR will add the implementation for streaming that should provide same response byte-to-byte.
Used test cases from: * Original PR kubernetes#129334 * KEP https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/5116-streaming-response-encoding#unit-tests For now testing current serializer implementation to show encoder behavior and agree on set of tests. Having a separate PR should make review easier. In separate PR will add the implementation for streaming that should provide same response byte-to-byte.
Used test cases from: * Original PR kubernetes#129334 * KEP https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/5116-streaming-response-encoding#unit-tests For now testing current serializer implementation to show encoder behavior and agree on set of tests. Having a separate PR should make review easier. In separate PR will add the implementation for streaming that should provide same response byte-to-byte.
Used test cases from: * Original PR kubernetes#129334 * KEP https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/5116-streaming-response-encoding#unit-tests For now testing current serializer implementation to show encoder behavior and agree on set of tests. Having a separate PR should make review easier. In separate PR will add the implementation for streaming that should provide same response byte-to-byte.
|
@anson627 @haosdent Thanks for taking initiative, however as described in #129334 (comment) the work has grown beyond just this PR. The work for streaming encoding was upgraded from a single PR to a full KEP. To understand the next steps you can read the design https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/5116-streaming-response-encoding/README.md and follow the implementation plan in kubernetes/enhancements#5116 |
|
thanks @serathius! I will take a closer look at the plan, and see where and how I can help |
e726aaa to
3804115
Compare
|
Updated PR, still need to wait for merging: |
|
The PR is ready for re-review @liggitt |
|
/retest |
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: 12bd3e248c3fe40c2d548ba8821c72b363229964 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fuweid, liggitt, 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 |
|
we tested in our env: list 6000 configmaps with size 150kb Conclusion： under json list streaming conditions, gzip may be more time-consuming before the mr #130281 after the mr #130281 |
|
10% difference seems ok. Our goal was to reduce memory usage, doing so is already a tradeoff that increases CPU usage. |
Agree, users could toggle off this if they don't want to compromise on latency |
I agree with this tradeoff |
/kind feature
Implements proposal from #129304 where streaming json respones reduced the memory used by apiserver 3-6x times. Validated on configmaps and pods objects with 5 concurrent list each returning 1.5GB of data.
/cc @mborsz @wojtek-t @jpbetz @deads2k @p0lyn0mial