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

why encode object instead of event and returned directly in watchEncoder? #122153

Closed
likakuli opened this issue Dec 1, 2023 · 6 comments
Closed
Labels
kind/support Categorizes issue or PR as a support question. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@likakuli
Copy link
Contributor

likakuli commented Dec 1, 2023

What happened?

https://github.com/kubernetes/kubernetes/pull/120300/files#diff-12e0758457373aa860bb0baae0878a99c107840d25fcf356d126d4b3d1d15663R177-R178

Encode function of watchEncoder write event serialization result to response. the above lines seems only write object instead of event to response. not sure if my understanding is correct.

What did you expect to happen?

write event to response

How can we reproduce it (as minimally and precisely as possible)?

add a unit test and set object in event to CacheableObject

Anything else we need to know?

No response

Kubernetes version

$ kubectl version
# paste output here

Cloud provider

OS version

# On Linux:
$ cat /etc/os-release
# paste output here
$ uname -a
# paste output here

# On Windows:
C:\> wmic os get Caption, Version, BuildNumber, OSArchitecture
# paste output here

Install tools

Container runtime (CRI) and version (if applicable)

Related plugins (CNI, CSI, ...) and versions (if applicable)

@likakuli likakuli added the kind/bug Categorizes issue or PR as related to a bug. label Dec 1, 2023
@k8s-ci-robot k8s-ci-robot added needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Dec 1, 2023
@k8s-ci-robot
Copy link
Contributor

@likakuli: The label(s) sig/apiserver cannot be applied, because the repository doesn't have them.

In response to this:

/sig apiserver

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/test-infra repository.

@likakuli
Copy link
Contributor Author

likakuli commented Dec 1, 2023

/sig api-machinery

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 1, 2023
@likakuli likakuli changed the title why encode object and returned directly in watchEncoder? why encode object instead of event and returned directly in watchEncoder? Dec 1, 2023
@liggitt liggitt added kind/support Categorizes issue or PR as a support question. and removed kind/bug Categorizes issue or PR as related to a bug. labels Dec 2, 2023
@liggitt
Copy link
Member

liggitt commented Dec 2, 2023

@wojtek-t could probably answer this

@wojtek-t
Copy link
Member

wojtek-t commented Dec 4, 2023

https://github.com/kubernetes/kubernetes/pull/120300/files#diff-12e0758457373aa860bb0baae0878a99c107840d25fcf356d126d4b3d1d15663R177-R178

Encode function of watchEncoder write event serialization result to response. the above lines seems only write object instead of event to response. not sure if my understanding is correct.

That is not correct - this works as it should.
You can take a look at #120300 for the implementation.

Basically, we store also encoding of the "whole watch event" inside the CachingObject cache to avoid serializing them independently. You can see that the identifier contains the watch event type so that each even would be serialized independently.
Appropriate tests were added in that PR too.

@likakuli
Copy link
Contributor Author

likakuli commented Dec 4, 2023

I misunderstood the code previously, but now it seems fine. Thank you

@likakuli likakuli closed this as completed Dec 4, 2023
@alexzielenski
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/support Categorizes issue or PR as a support question. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

5 participants