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.

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

Import Thanos protobuf definitions #3222

Merged
merged 7 commits into from
Oct 17, 2022
Merged

Import Thanos protobuf definitions #3222

merged 7 commits into from
Oct 17, 2022

Conversation

pracucci
Copy link
Collaborator

@pracucci pracucci commented Oct 14, 2022

What this PR does

Mimir and Thanos store-gateways have diverged (e.g. query sharding) and, to move forward, we need to import Thanos protobuf definitions into Mimir. In this PR I'm doing it.

As separate commits:

Note to reviewers:

  • I kept the Thanos protobuf go packages as is (e.g. storepb, labelpb, ...) in order to keep the diff minimal and make it easier to review. At first I did merged all of them into our storegatewaypb but diff was too difficult to review. We can consider merging them as a follow up work.
  • I introduced an integration test to ensure store-gateway backward compatibility (we didn't have one). To prove the new integration test works, I intentionally pushed a commit to introduce a breaking change in the store-gateway protobuf 762b146 and its CI failed. Then I rolled it back.

Which issue(s) this PR fixes or relates to

N/A

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@pracucci pracucci requested a review from a team as a code owner October 14, 2022 19:23
@pracucci pracucci mentioned this pull request Oct 14, 2022
3 tasks
@pracucci pracucci marked this pull request as draft October 14, 2022 19:23
@pracucci pracucci force-pushed the import-thanos-pb branch 5 times, most recently from 762b146 to 70cfc41 Compare October 15, 2022 12:59
@pracucci pracucci marked this pull request as ready for review October 15, 2022 18:26
Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Very good overall. Few nits and one thought for further code elimination.

Makefile Outdated Show resolved Hide resolved
@@ -20,6 +20,7 @@ import (
"github.com/grafana/dskit/kv"
"github.com/grafana/dskit/ring"
"github.com/grafana/dskit/services"
"github.com/grafana/dskit/tenant"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: moving this line is not relevant to this commit

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We're used to group imports, and we typically fix it lazy the imports are not grouped. It's a practice we adopt regularly. I will not address this.

pkg/storegateway/bucket.go Outdated Show resolved Hide resolved
@@ -21,21 +21,20 @@ import (
"github.com/prometheus/prometheus/model/timestamp"
"github.com/prometheus/prometheus/tsdb/hashcache"
"github.com/stretchr/testify/assert"
"github.com/thanos-io/objstore"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit again, changes not relevant to this commit

Comment on lines +33 to +34
func (m *Label) Reset() { *m = Label{} }
func (*Label) ProtoMessage() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

This Label type is identical to LabelPair here:

type LabelPair struct {
Name []byte `protobuf:"bytes,1,opt,name=name,proto3" json:"name,omitempty"`
Value []byte `protobuf:"bytes,2,opt,name=value,proto3" json:"value,omitempty"`
}

Mimir has LabelAdapter to do the zero-copy thing where Thanos has ZLabel.

(I would be ok with doing that change later, after this PR)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will look at it separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

I replaced ZLabel with LabelAdapter in #3345.

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants