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

Use new reverse protobuf marshalling #77355

Merged
merged 5 commits into from Jul 25, 2019

Conversation

@apelisse
Copy link
Member

commented May 2, 2019

This is trying a new version of gogo/protobuf serialization that goes backward. It's still a WIP, especially since the code is not merged in that repo, so we're replacing in go.mod to point to the pull-request instead. Let's see if this works.

What type of PR is this?
/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #76219

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Update gogo/protobuf to serialize backward, as to get better performance on deep objects.
@mattjmcnaughton

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

Very cool stuff @apelisse :) I'm curious about your test plans for this - how do you plan to verify the performance improvement? In this pr or in a separate pr?

Thanks again for all your work on this :)

@apelisse

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

how do you plan to verify the performance improvement? In this pr or in a separate pr?

We are planning to test that PR on the scalability cluster with 5000 nodes and see what happens!

@apelisse apelisse force-pushed the apelisse:test-new-protoc branch from d24963f to 9d46a7a May 3, 2019

@apelisse

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

Rebased, let's see how it goes

@k8s-ci-robot k8s-ci-robot removed the size/S label May 3, 2019

@wojtek-t

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

/retest

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 25, 2019

@smarterclayton

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

/approve

@apelisse apelisse force-pushed the apelisse:test-new-protoc branch from c0138d9 to 6568325 Jul 25, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jul 25, 2019

@wojtek-t

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 25, 2019

@apelisse

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

OK I fixed the comment and squashed a fixup commit.

@BenTheElder

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

/assign @yujuhong @Random-Liu
PTAL for cri-api (the change is just proto optimization, not API changes)

@tallclair

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

/approve for CRI changes

@tallclair

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

er
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, smarterclayton, tallclair, wojtek-t

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@BenTheElder

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

/retest
pod pending timeout from prow 🙄 on pull-kubernetes-bazel-test

@k8s-ci-robot k8s-ci-robot merged commit a172e19 into kubernetes:master Jul 25, 2019

23 checks passed

cla/linuxfoundation apelisse authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@@ -64,7 +64,7 @@ require (
github.com/go-openapi/validate v0.19.2
github.com/go-ozzo/ozzo-validation v3.5.0+incompatible // indirect
github.com/godbus/dbus v4.1.0+incompatible
github.com/gogo/protobuf v1.0.0
github.com/gogo/protobuf v0.0.0-20190410021324-65acae22fc9

This comment has been minimized.

Copy link
@liggitt

liggitt Jul 26, 2019

Member

was there a tag that included this change? when possible, we're trying to stick to tagged versions of dependencies to be sure we don't pick up half-working releases

This comment has been minimized.

Copy link
@smarterclayton

smarterclayton Jul 26, 2019

Contributor

Can we make that a follow up? If we hit an issue I’d rather tag in gogo once we’re certain this works.

This comment has been minimized.

Copy link
@smarterclayton

smarterclayton Jul 26, 2019

Contributor

Follow up == before 1.16

This comment has been minimized.

Copy link
@liggitt

liggitt Jul 26, 2019

Member

sure, added back to #79234

This comment has been minimized.

Copy link
@apelisse

apelisse Jul 26, 2019

Author Member

I used the hash of the merge commit for the PR that we wanted.
@jmarais, Should we expect that to be a "half-working" release? Do you know when is the next release that will include the reverse marshaling? Thanks!

This comment has been minimized.

Copy link
@dims

dims Jul 29, 2019

Member

@smarterclayton @apelisse looks like this survived for 3 days or so. Wanna created a tag in gogo/protobuf please?

This comment has been minimized.

Copy link
@apelisse

apelisse Jul 29, 2019

Author Member

We don't own that repo, I'll follow-up with the owner

@apelisse

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

Hard to know what really is going on yet, but that's certainly related: gogo/protobuf#589

@dims dims referenced this pull request Aug 4, 2019

Open

Request for a release/tag #597

@liggitt liggitt added this to the v1.16 milestone Aug 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.