-
Notifications
You must be signed in to change notification settings - Fork 512
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
Remove Thanos ZLabel type in favour of identical LabelAdapter type #3345
Conversation
@@ -31,7 +31,7 @@ message Chunk { | |||
} | |||
|
|||
message Series { | |||
repeated Label labels = 1 [(gogoproto.nullable) = false, (gogoproto.customtype) = "github.com/grafana/mimir/pkg/storegateway/labelpb.ZLabel"]; | |||
repeated cortexpb.LabelPair labels = 1 [(gogoproto.nullable) = false, (gogoproto.customtype) = "github.com/grafana/mimir/pkg/mimirpb.LabelAdapter"]; |
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.
Why do we need the change here to cortexpb.LabelPair
?
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.
Because I deleted Label.
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.
Oh it was in types.proto. Sorry I missed that one.
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.
So the proto will unmarshal even if the type name is different? In that case, should we create a task to rename cortexpb
to mimirpb
?
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.
I added more info to the PR description.
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.
LGTM!
The implementations are identical.
Avoid having two names for the same thing.
So everything compiles and lint errors go away.
…rafana#3345) * labelpb: remove unused LabelSet type * Replace ZLabel with alias to LabelAdapter The implementations are identical. * Remove unused storegateway/prompb * storegateway: convert uses of ZLabel to LabelAdapter Avoid having two names for the same thing. * go mod tidy: xxhash dependency is now indirect * Make imports of gogo.proto consistent So everything compiles and lint errors go away.
…rafana#3345) * labelpb: remove unused LabelSet type * Replace ZLabel with alias to LabelAdapter The implementations are identical. * Remove unused storegateway/prompb * storegateway: convert uses of ZLabel to LabelAdapter Avoid having two names for the same thing. * go mod tidy: xxhash dependency is now indirect * Make imports of gogo.proto consistent So everything compiles and lint errors go away.
This code was brought in with #3222, but exactly duplicated code we already had, except for the name.
(
LabelAdapter
isn't a hugely descriptive name, but it was widely used already)All the types in
storegateway/labelpb
were removed:Label
,LabelSet
andZLabelSet
; the last two were entirely unused in Mimir.Label
was equivalent toLabelPair
in Mimir, albeit usingstring
instead ofbytes
type. According to here these are identical on the wire, butstring
should only contain valid UTF-8 data. (This is not checked in Go).Checklist
CHANGELOG.md
updated - not user-facing