-
Notifications
You must be signed in to change notification settings - Fork 39.1k
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
Proposal for introducing Protobuf serialization #22600
Proposal for introducing Protobuf serialization #22600
Conversation
@kubernetes/sig-api-machinery @wojtek-t |
Labelling this PR as size/L |
526590d
to
d416b56
Compare
Labelling this PR as size/XL |
GCE e2e build/test passed for commit 6601d4a1f4ef56f9728db62e5960dcf4dbeea1c8. |
GCE e2e build/test passed for commit 526590db385ab8839f69d1e398d65a3165252e10. |
GCE e2e build/test passed for commit d416b56a5f594be6ae7da1948cb211d70cb8028d. |
2x speed up vs the 10x observed for Protobuf | ||
* gRPC was considered, but is a larger change that requires more core | ||
refactoring. This approach does not eliminate the possibility of switching | ||
to gRPC in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I think it will even be easier if we decide to do it in the future.
@smarterclayton - very nice doc @smarterclayton - thanks a lot for writing that down! |
I just added some comments, but those are mainly to ensure that I understood it correctly. LGTM, but probably more people should look into it before merging. |
Just one more point from me to incorporate your answer about tags to your doc. |
d416b56
to
ebafdcc
Compare
GCE e2e build/test passed for commit ebafdccb059907ec7163dfd92cc7cbd54d5d8f9a. |
* Protobuf objects on disk or in etcd will need to be self identifying at | ||
rest, like JSON, in order for backwards compatibility in storage to work, | ||
so we must add an envelope with apiVersion and kind to wrap the nested | ||
object, and make the data format recognizable to clients. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, and encoding? So we can say e.g. compressed, encrypted, maybe even json or yaml (for emergency escape hatch).
LGTM - a few very minor nits. |
|
||
## Implications | ||
|
||
* The generated marshal code is large and will increase build times and binary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just out of curiosity how much worse is build time and binary size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About 15-25%.
Pushed updates.
On Wed, Mar 16, 2016 at 12:23 AM, Derek Carr notifications@github.com
wrote:
In docs/proposals/protobuf.md
#22600 (comment)
:+handled using our existing API change rules. Tags cannot change within an
+API version.
+
+Generation would be done by developers and then checked into source control,
+like conversions and ugorji JSON codecs.
+
+Because protoc is not packaged well across all platforms, we will add it to
+thekube-cross
Docker image and developers can use that to generate
+updated protobufs. Protobuf 3 beta is required.
+
+The generated protobuf will be checked with a verify script before merging.
+
+
+## Implications
+
+* The generated marshal code is large and will increase build times and binaryJust out of curiosity how much worse is build time and binary size?
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/22600/files/ebafdccb059907ec7163dfd92cc7cbd54d5d8f9a#r56280142
ebafdcc
to
252b2fa
Compare
GCE e2e build/test passed for commit 252b2fa9ae501353b8512c79fe82e82b85ad4e65. |
LGTM, but tests need attention |
252b2fa
to
2234c13
Compare
PR changed after LGTM, removing LGTM. |
GCE e2e build/test passed for commit 2234c138dde2069f73c1807a1fac7a2d42ce7325. |
2234c13
to
95cf60b
Compare
GCE e2e build/test passed for commit 95cf60b. |
Fixed docs, reapplying lgtm |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
GCE e2e build/test passed for commit 95cf60b. |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
GCE e2e build/test passed for commit 95cf60b. |
Automatic merge from submit-queue |
Auto commit by PR queue bot
Auto commit by PR queue bot
For the 1.3 timeframe.
Current status:
hack/update-generated-protobuf.sh
today)ContentType
header and server should support it