[GarbageCollector] only store typeMeta and objectMeta in the gc store #28480
Conversation
aae495e to
a4f19d2
Compare
There was a problem hiding this comment.
What's the behavior if no codec is provided?
It seems like there is a default Codec. Would you like to add a comment for it?
There was a problem hiding this comment.
I thought there was a codec in the restclient config-- if not, consider adding it there instead of adding a parameter here?
There was a problem hiding this comment.
dynamic.NewClient ignored it and overrode it with the dynamic codec.
There was a problem hiding this comment.
I actually made dynamic.NewClient to respect the config. I'll see if I can squeeze the codec into the config.
a4f19d2 to
cca9579
Compare
|
My point was that unrecognized types is the current edge case, and for The thing is the payload of object meta can change, and will change at some That's not a "I think this is completely wrong", but it is a "this is On Mon, Jul 25, 2016 at 5:59 PM, k8s-merge-robot notifications@github.com
|
There was a problem hiding this comment.
I think the danger is that MetaOnly won't work if we use incompatible ObjectMeta across different groups in the future. @smarterclayton @lavalamp is this going to happen in the near future?
There was a problem hiding this comment.
We don't have a mechanism to convert TPR or unrecognized types anyway.
Which means that swagger and/or api discovery has to communicate that info
if we do have to solve that. By putting this in we probably aren't making
that problem worse... except for our own types.
On Mon, Jul 25, 2016 at 11:20 PM, Chao Xu notifications@github.com wrote:
In pkg/controller/garbagecollector/metaonly/metaonly.go
#28480 (comment)
:
- "io"
- "reflect"
- "strings"
- "k8s.io/kubernetes/pkg/api/unversioned"
- "k8s.io/kubernetes/pkg/api/v1"
- "k8s.io/kubernetes/pkg/runtime"
- "k8s.io/kubernetes/pkg/util/json"
+)
+// MetaOnly allows decoding only the apiVersion, kind, and metadata fields of
+// JSON data.
+// TODO: enable meta-only decoding for protobuf.
+type MetaOnly struct {
- unversioned.TypeMeta
json:",inline"- v1.ObjectMeta
json:"metadata,omitempty"I think the danger is that MetaOnly won't work if we use incompatible
ObjectMeta across different groups in the future. @smarterclayton
https://github.com/smarterclayton @lavalamp
https://github.com/lavalamp is this going to happen in the near future?—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/kubernetes/pull/28480/files/cca957905d7ed2af91d0444ecb3dd717ac8c9825#r72182360,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p3ws2Zajl7FyzwA3YwRNuGEp5X4iks5qZXzpgaJpZM4JEvyT
.
cca9579 to
82f14e8
Compare
8e57ca5 to
41d92b1
Compare
| if simpleStorage.deleted != ID { | ||
| t.Errorf("Unexpected delete: %s, expected %s", simpleStorage.deleted, ID) | ||
| } | ||
| simpleStorage.deleteOptions.GetObjectKind().SetGroupVersionKind(unversioned.GroupVersionKind{}) |
There was a problem hiding this comment.
These changes are needed because I change the UseOrCreateObject. The decoder uses the passed in into object instead of creating a new one. In turn the versioning decoder skips the conversion so the gvk is kept. I think this behavior is an innocuous bug.
|
I think this is close, @caesarxuchao if you can address the comments here I think we can get this in soon? |
|
@lavalamp Comments addressed. PTAL. |
|
|
||
| // PrintAsMetadataOnlyObject is a helper function that converts an interface{} to | ||
| // *MetadataOnlyObject and then convert it to a human-readable string. | ||
| func PrintAsMetadataOnlyObject(obj interface{}) string { |
There was a problem hiding this comment.
Is this function needed at all? Looking at the call sites, I'm pretty sure you can just get rid of it.
There was a problem hiding this comment.
Right. I'll remove it.
|
just the one nit. Please fix & squash, the apply the label. Thanks! |
d76df0d to
4d23506
Compare
|
Thanks. Squashed and self applied the label per comment. |
|
GCE e2e build/test passed for commit 4d23506. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit 4d23506. |
|
Automatic merge from submit-queue |
Automatic merge from submit-queue (batch tested with PRs 64069, 64087). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. garbage collector `NewMetadataCodecFactory` cleanup `NewMetadataCodecFactory` was introduce by #28480, but not used now. /assign @deads2k **Release note**: ```release-note NONE ```
GC only needs to know the apiVersion, kind, and objectMeta of an object. This PR makes the stores of GC only save these fields.
cc @kubernetes/sig-api-machinery
This change is