Skip to content

Conversation

@pracucci
Copy link
Contributor

@pracucci pracucci commented Apr 7, 2020

What this PR does:
The upcoming store-gateway (proposal) will run the Thanos BucketStore instead of the querier.

Currently, we do have a multi-tenant BucketStore wrapper which is UserStore. The UserStore is both a wrapper over BucketStore and a service which does an initial blocks synching + a periodic sync.

The store-gateway will need to trigger the sync differently due to the sharding. In particular, the store-gateway needs to coordinate the initial sync while the instance is in the JOINING state within the ring (because it needs to have the tokens first) and to trigger a sync both periodically and when the token ranges assigned to instance itself change (ring topology changed).

Instead of duplicating code between the querier and store-gateway for the co-existence period, in this PR I'm decoupling the multi-tenant wrapper of BucketStore from UserStore:

  • BucketStores: multi-tenant wrapper over BucketStore (will be directly used by the store-gateway). The Series() API here matches the exact gRPC service signature of the upcoming store-gateway so that we can do a pass-through to the Thanos BucketStore
  • UserStore: BucketStores + service. I haven't renamed it in this PR to keep the diff easier. Also UserStore will disappear once the store-gateway will be complete.

Which issue(s) this PR fixes:
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 pstibrany April 7, 2020 14:48
@pracucci pracucci force-pushed the refactor-bucket-stores-for-store-gateway branch 2 times, most recently from 46adc64 to 59b3694 Compare April 7, 2020 15:00
Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

LGTM.

UserStore: BucketStores + service. I haven't renamed it in this PR to keep the diff easier. Also UserStore will disappear once the store-gateway will be complete.

Feel free to rename, it's in its own file now anyway, diff will not be much bigger I think.

Unrelated to this PR, but perhaps worth fixing... should function used in Iter call react on context being done?

		select {
		case jobs <- job{userID: user, store: bs}:
			return nil
		case <-ctx.Done():
			return ctx.Err()
		}

Otherwise, it can be blocked here for some time, until some worker is finished with sync.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it will. I think deduplicate filter removes old blocks if new blocks cover it completely, but we better verify it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we may have troubles with the consistency check, at least as I did design it. I will write down different scenarios in tests and let's see (once we'll reach the point to build the consistency check).

@pracucci
Copy link
Contributor Author

pracucci commented Apr 8, 2020

Unrelated to this PR, but perhaps worth fixing... should function used in Iter call react on context being done?

Definitely.

@pracucci
Copy link
Contributor Author

pracucci commented Apr 8, 2020

Feel free to rename, it's in its own file now anyway, diff will not be much bigger I think.

As agreed offline, renamed UserStore to BucketStoresService.

@pracucci pracucci force-pushed the refactor-bucket-stores-for-store-gateway branch from 59b3694 to ea54e44 Compare April 8, 2020 12:41
@pracucci pracucci merged commit 50f53db into cortexproject:master Apr 8, 2020
@pracucci pracucci deleted the refactor-bucket-stores-for-store-gateway branch April 8, 2020 13:01
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.

2 participants