feat: add switch control when watching events whether deserialization… #2369
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@Kevinz857: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
kind/feature
|
Welcome @Kevinz857! |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Kevinz857 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/XS
| event = self.unmarshal_event(line, return_type) | ||
| else: | ||
| # Only do basic JSON parsing, no deserialize | ||
| event = json.loads(line) |
There was a problem hiding this comment.
Here event['raw_object'] is not populated like here does, which means the retry_after_410 logic won't work.
We need to make sure the retry logic continue to work even when we don't deserialize the object
| while True: | ||
| resp = func(*args, **kwargs) | ||
| try: | ||
| for line in iter_resp_lines(resp): | ||
| # unmarshal when we are receiving events from watch, | ||
| # return raw string when we are streaming log | ||
| if watch_arg == "watch": | ||
| event = self.unmarshal_event(line, return_type) | ||
| if deserialize: | ||
| event = self.unmarshal_event(line, return_type) |
There was a problem hiding this comment.
We actually have the return_type flag which could serve the purpose, but it looks pretty unintuitive-- IIUC _raw_return_type can be persisted by setting return_type to False, which would result in deserialization being skipped
That being said, the flag name seems unintuitive and I think deserialize = False makes more sense.
cc @yliaog
| event = self.unmarshal_event(line, return_type) | ||
| else: | ||
| # Only do basic JSON parsing, no deserialize | ||
| event = json.loads(line) |
There was a problem hiding this comment.
I also noticed that in this way, the resource version is not tracked like here does.
I think we can refactor the unmarshal_event method to keep track of the resource version even when we skip deserialization.
| event = self.unmarshal_event(line, return_type) | ||
| else: | ||
| # Only do basic JSON parsing, no deserialize | ||
| event = json.loads(line) |
There was a problem hiding this comment.
Please update the tests to ensure the retry and resource version behaviors when deserialize = False
k8s-ci-robot
added
size/M
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What type of PR is this?
/kind feature
/kind optimization
What this PR does / why we need it:
This PR adds an option to disable deserialization in the Watch.stream() method to improve performance for high-frequency watch operations. When dealing with large-scale Kubernetes clusters or high-frequency resource updates, the default deserialization of every event can cause significant latency overhead and potential processing delays.
By allowing clients to opt-out of automatic deserialization when only basic JSON parsing is needed, we can significantly reduce time cost and improve event processing throughput. This is particularly important in scenarios with high event volumes or resource constraints.
Key changes:
deserializeparameter toWatch.stream()method (defaults to True for backward compatibility)deserialize=False, events are only JSON parsed without model conversiondeserialize=TrueWhich issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
The change is backward compatible and doesn't affect existing code. Users can opt-in to the performance optimization by explicitly setting
deserialize=Falsewhen they don't need the full model objects.Performance improvement example: