Add a test to validate deferredResponseWriter on multiple writes by nkeert · Pull Request #130190 · kubernetes/kubernetes

nkeert · 2025-02-15T05:40:34Z

What type of PR is this?

What this PR does / why we need it:

Thie PR adds test for deferredResponseWriter as requested in #130168

Which issue(s) this PR fixes:

Fixes #130168

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

linux-foundation-easycla · 2025-02-15T05:40:37Z

The committers listed above are authorized under a signed CLA.

✅ login: nkeert / name: NKeert (45e2f3e)

k8s-ci-robot · 2025-02-15T05:40:42Z

Welcome @nkeert!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-02-15T05:40:43Z

Hi @nkeert. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

serathius · 2025-02-16T11:01:08Z

+					t.Fatalf("failed to decompress: %v", err)
+				}
+
+				if len(decompressed) != tt.resSize {


Can you compare contents instead of just size?

sure, I have update the PR with the suggestion.

serathius · 2025-02-16T11:05:25Z

Looks great! Thanks for fast response.

/ok-to-test
cc @liggitt

serathius · 2025-02-17T08:52:36Z

/retest

serathius · 2025-02-17T08:53:38Z

+		name       string
+		chunks     [][]byte
+		expectGzip bool
+		resSize    int


Don't think testing resSize is needed anymore.

Thanks for the review. I have updated the PR.

serathius · 2025-02-17T15:57:45Z

/lgtm

/assign @liggitt @deads2k

k8s-ci-robot · 2025-02-17T15:57:53Z

LGTM label has been added.

Details

Git tree hash: 45c22fef75c75b44415e59a51d5b9b729a15b184

Signed-off-by: nkeert <197718357+nkeert@users.noreply.github.com>

nkeert · 2025-02-17T23:54:22Z

/retest

fuweid

LGTM(nb)!

serathius · 2025-02-18T08:12:54Z

/retest

liggitt

/lgtm
/approve

liggitt · 2025-02-18T18:51:25Z

+		{
+			name:       "two small chunk writes",
+			chunks:     [][]byte{smallChunk, smallChunk},
+			expectGzip: false,


just leaving a comment here that we consider this to be sort of a bug, so that when we change the implementation in the future and fix this case, editing the unit test at the same time will not be surprising in the diff

k8s-ci-robot · 2025-02-18T20:04:12Z

LGTM label has been added.

Details

Git tree hash: 3f516ee30accd341287d0314ba0fd6b30048070b

k8s-ci-robot · 2025-02-18T20:04:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fuweid, liggitt, nkeert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/apiserver/pkg/endpoints/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cici37 · 2025-02-18T21:24:18Z

/triage accepted

serathius · 2025-02-19T08:24:30Z

/kind feature

serathius · 2025-02-19T08:30:03Z

/release-note-none

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 15, 2025

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Feb 15, 2025

k8s-ci-robot requested review from enj and tkashem February 15, 2025 05:40

nkeert mentioned this pull request Feb 15, 2025

Implement tests for deferredResponseWriter to validate behavior on multiple writer calls. #130168

Closed

nkeert changed the title ~~Add a test to validate deferredResponseWriteron multiple write calls~~ Add a test to validate deferredResponseWriter on multiple writes Feb 15, 2025

serathius reviewed Feb 16, 2025

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 16, 2025

nkeert force-pushed the test-validate-deferredResponseWriter-for-multiple-writes branch 3 times, most recently from 817a620 to f35ddf8 Compare February 16, 2025 12:39

serathius reviewed Feb 17, 2025

View reviewed changes

k8s-ci-robot assigned serathius Feb 17, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 17, 2025

Add a test to validate deferredResponseWriteron multiple write calls

45e2f3e

Signed-off-by: nkeert <197718357+nkeert@users.noreply.github.com>

nkeert force-pushed the test-validate-deferredResponseWriter-for-multiple-writes branch from f35ddf8 to 45e2f3e Compare February 17, 2025 23:53

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 17, 2025

k8s-ci-robot requested review from deads2k, liggitt and serathius February 17, 2025 23:53

fuweid approved these changes Feb 18, 2025

View reviewed changes

liggitt reviewed Feb 18, 2025

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 18, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 18, 2025

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 18, 2025

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Feb 19, 2025

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 19, 2025

serathius mentioned this pull request Feb 19, 2025

Implement chunking for gzip encoder in deferredResponseWriter #130264

Closed

k8s-ci-robot merged commit 7fc8a86 into kubernetes:master Feb 19, 2025

k8s-ci-robot added this to the v1.33 milestone Feb 19, 2025

pacoxu mentioned this pull request Mar 7, 2025

[v1.34]promote APIResponseCompression to GA #129748

Closed

Conversation

nkeert commented Feb 15, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

linux-foundation-easycla Bot commented Feb 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Feb 15, 2025

Uh oh!

k8s-ci-robot commented Feb 15, 2025

Uh oh!

serathius Feb 16, 2025

Choose a reason for hiding this comment

Uh oh!

nkeert Feb 16, 2025

Choose a reason for hiding this comment

Uh oh!

serathius commented Feb 16, 2025

Uh oh!

serathius commented Feb 17, 2025

Uh oh!

serathius Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

nkeert Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

serathius commented Feb 17, 2025

Uh oh!

k8s-ci-robot commented Feb 17, 2025

Uh oh!

nkeert commented Feb 17, 2025

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

serathius commented Feb 18, 2025

Uh oh!

liggitt left a comment

Choose a reason for hiding this comment

Uh oh!

liggitt Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Feb 18, 2025

Uh oh!

k8s-ci-robot commented Feb 18, 2025

Uh oh!

cici37 commented Feb 18, 2025

Uh oh!

serathius commented Feb 19, 2025

Uh oh!

serathius commented Feb 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

linux-foundation-easycla Bot commented Feb 15, 2025 •

edited

Loading