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.

Sign up for GitHub

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

Jump to bottom

Add direct serializer #26251

Merged
merged 1 commit into from
Jun 2, 2016
Merged

Add direct serializer #26251

merged 1 commit into from
Jun 2, 2016

Conversation

caesarxuchao
Copy link
Member

@caesarxuchao caesarxuchao commented May 25, 2016 •
edited
Loading

Fix #25589. Implemented a direct codec that doesn't do conversion, but sets the group, version and kind before serialization as Clayton suggested here.

First commit is cherry-picked from #24826.

@kubernetes/sig-api-machinery

@caesarxuchao caesarxuchao added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label May 25, 2016
@caesarxuchao caesarxuchao added this to the v1.3 milestone May 25, 2016
@caesarxuchao caesarxuchao self-assigned this May 25, 2016
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels May 25, 2016
@caesarxuchao caesarxuchao added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note-label-needed labels May 25, 2016
@caesarxuchao caesarxuchao changed the title [WIP] Add direct serializer Add direct serializer May 25, 2016
caesarxuchao
caesarxuchao reviewed May 25, 2016
View reviewed changes
pkg/runtime/serializer/codec_factory.go
@@ -308,3 +310,52 @@ func (f CodecFactory) SerializerForFileExtension(extension string) (runtime.Seri
}
return nil, false
}

type DirectCodecFactory struct {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smarterclayton these are the important bits of this PR. Others are mostly generated code.

@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 25, 2016
smarterclayton
smarterclayton reviewed May 25, 2016
View reviewed changes
pkg/runtime/serializer/codec_factory.go

func (f DirectCodecFactory) EncoderForVersion(serializer runtime.Encoder, gv unversioned.GroupVersion) runtime.Encoder {
return DirectCodec{
struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not use anonymous structs. Also, there should be a struct for this already in pkg/runtime/codecs.go

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Called runtime.NewCodec().

@smarterclayton
Copy link
Contributor

A few comments

@caesarxuchao
Copy link
Member Author

@smarterclayton comments addressed, PTAL. Thanks.

@caesarxuchao
Copy link
Member Author

I'll fix the go-vet issues after get the lgtm.

smarterclayton
smarterclayton reviewed May 26, 2016
View reviewed changes
pkg/runtime/serializer/codec_factory.go
kind.SetGroupVersionKind(gvk)
defer kind.SetGroupVersionKind(oldGVK)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be better not to defer here to avoid an allocation (this will be in the critical path for some code).

err = c.Serializer.EncodeToStream(obj, stream, overrides...)
kind.SetGroupVersionKind(oldGVK)
return err

I eliminated the defer in another PR for versioning codecs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I wasn't aware of that defer is costly. PTAL. Thanks!

@smarterclayton
Copy link
Contributor

Changes look good, if you can add some minimal Godoc this is LGTM and you can self apply.

@caesarxuchao
Copy link
Member Author

Oh sure. Thanks.

@caesarxuchao
Copy link
Member Author

Godocs added. Applying LGTM per comment.

@caesarxuchao caesarxuchao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2016
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 26, 2016
@caesarxuchao caesarxuchao added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 26, 2016
@caesarxuchao
Copy link
Member Author

Added back the lgtm after trivial rebase.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2016
@caesarxuchao
Copy link
Member Author

@k8s-bot node e2e test this, issue #26385

1 similar comment
@caesarxuchao
Copy link
Member Author

@k8s-bot node e2e test this, issue #26385

@wojtek-t
Copy link
Member

@caesarxuchao - it seems that something doesn't compile now. Can you please fix?

@caesarxuchao
Copy link
Member Author

Rebased. ObjectKind() method is removed, I used the first gvk returned by ObjectKinds() instead.

@caesarxuchao caesarxuchao added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 31, 2016
@smarterclayton
Copy link
Contributor

Change looks fine.

@caesarxuchao
Copy link
Member Author

Thanks for the confirmation.

@caesarxuchao
Copy link
Member Author

@k8s-bot unit test this, issue #12312

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Jun 2, 2016

GCE e2e build/test passed for commit f32f396.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit cf234ab into kubernetes:master Jun 2, 2016
@nikhiljindal nikhiljindal mentioned this pull request Jun 2, 2016
Closed
@erictune erictune mentioned this pull request Jul 2, 2016
Closed
@erictune
Copy link
Member

erictune commented Jul 2, 2016

@caesarxuchao Does this PR require action by the user when upgrading from 1.2.x to 1.3.0? (Think about non-developer users.) If so, please edit your first comment to have a release-note block, like in #28132. If it is just an optional feature, please change the label to just release-note. If it is not a complete feature by itself, then apply "release-note-none" label instead.

@caesarxuchao caesarxuchao added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. labels Jul 2, 2016
@caesarxuchao caesarxuchao deleted the add-serializer branch May 12, 2020 23:10
@enj enj mentioned this pull request Jul 12, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@smarterclayton smarterclayton

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.
7 participants
@caesarxuchao @smarterclayton @wojtek-t @k8s-github-robot @k8s-bot @erictune @googlebot