atepg(cleanup): update the in-place metadata setter migration - #1377
atepg(cleanup): update the in-place metadata setter migration#1377shrutiyam-glitch wants to merge 4 commits into
Conversation
Tim Hockin (thockin)
left a comment
Tim Hockin (thockin)
left a comment
There was a problem hiding this comment.
I appreciate the cleanup, but I think this is being "too nice" about bad input.
| dbAtespace.Metadata = &ateapipb.ResourceMetadata{} | ||
| } | ||
| // Atespaces are global-scoped, so the atespace is always empty. | ||
| dbAtespace.Metadata.Atespace = "" |
There was a problem hiding this comment.
This should have been validated already, right? If this is set and we get this far, there's a problem. Doing this here implies that this is acceptable, and I think it's impossible?
| dbAtespace.Metadata = newCreateMetadata("", name) | ||
| // The atespace is mutated in place: callers pass a dedicated object. | ||
| dbAtespace := atespace | ||
| if dbAtespace.Metadata == nil { |
There was a problem hiding this comment.
Under what conditions would we end up here with nil metadata? If that is the case, we don't know the name of the atespace to create, we certainly can't proceed. This should have been validated already?
I see similar patterns elsewhere -- we should make them all consistent.
| dbAtespace := proto.Clone(atespace).(*ateapipb.Atespace) | ||
| dbAtespace.Metadata = newCreateMetadata("", name) | ||
| // The atespace is mutated in place: callers pass a dedicated object. | ||
| dbAtespace := atespace |
There was a problem hiding this comment.
Why not just get rid of dbAtespace and use atespace?
| // The actor is mutated in place: the caller already builds a dedicated | ||
| // object to pass in, so a defensive clone is wasted work. | ||
| dbActor := actor | ||
| if dbActor.Metadata == nil { |
There was a problem hiding this comment.
Same as elsewhere - we really CAN'T be in here without metadata because we don't have a name!
| dbTemplate := proto.Clone(template).(*ateapipb.ActorTemplate) | ||
| // The template is mutated in place: callers pass a dedicated object. | ||
| dbTemplate := template | ||
| if dbTemplate.Metadata == nil { |
There was a problem hiding this comment.
we really CAN'T be in here without metadata because we don't have a name!
2 participants
Storage-layer cleanup, follow-up to the DV migration PRs (#1250, #1244, #1303, #1261). No behavior changes visible to API clients.
Retire
newCreateMetadata/newUpdateMetadata. Both carried TODO: EOL this in favor ofset{Create,Update}Metadatafrom the Worker PR's review. The Worker paths were migrated there; this switches the remaining callers — atespace, egress policy, snapshot, and snapshot-tag creates/updates — to the in-place setters (with nil-metadata guards) and deletes the two helpers.Drop the clone in
CreateActor/CreateActorTemplate/CreateWorker. The only caller (ServiceImpl.Create<resource>) builds a dedicated object for the call and never reads it afterward, so the deep copy on every actor creation bought nothing.Tests pass
Appropriate changes to documentation are included in the PR