-
Notifications
You must be signed in to change notification settings - Fork 460
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
Make owned series service in ingester work with partitions ring. #7508
Make owned series service in ingester work with partitions ring. #7508
Conversation
|
||
oss := i.ownedSeriesService | ||
// | ||
// TODO: When using partitions ring, we need to update all tenants after partition is registered into the ring. That is done by partitionLifecycler's starting method. |
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 will send new PR for this.
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.
|
||
user string |
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.
moved ownedServiceTestUser
to TestOwnedSeriesService
test, because TestOwnedSeriesServiceWithPartitionsRing
needs different user, so that we can test some scenarios for moving series between partitions.
require.NoError(t, err) | ||
require.False(t, changed) | ||
}) | ||
func TestOwnedSeriesServiceWithPartitionsRing(t *testing.T) { |
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.
This is copy of TestOwnedSeriesService
test adapted to use partitions. I did not try to unify the tests, to avoid making this even more complicated.
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.
Agree 👍
} | ||
} | ||
|
||
func TestOwnedSeriesIngesterRingStrategyRingChanged(t *testing.T) { |
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.
This is renamed TestOwnedSeriesRingChanged
test.
}) | ||
} | ||
|
||
func TestOwnedSeriesPartitionsRingStrategyRingChanged(t *testing.T) { |
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.
This is TestOwnedSeriesIngesterRingStrategyRingChanged
adapted for partitions. Some test cases are different.
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.
Nice job! LGTM. I left few minor comments.
require.NoError(t, err) | ||
require.False(t, changed) | ||
}) | ||
func TestOwnedSeriesServiceWithPartitionsRing(t *testing.T) { |
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.
Agree 👍
return prepareIngesterWithBlockStorageAndOverridesAndPartitionRing(t, ingesterCfg, overrides, ingestersRing, nil, dataDir, bucketDir, registerer) | ||
} | ||
|
||
func prepareIngesterWithBlockStorageAndOverridesAndPartitionRing(t testing.TB, ingesterCfg Config, overrides *validation.Overrides, ingestersRing ring.ReadRing, partitionsRing *ring.PartitionRingWatcher, dataDir string, bucketDir string, registerer prometheus.Registerer) (*Ingester, error) { |
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.
Have you considered using createTestIngesterWithIngestStorage()
. If there's no hard blocker I would prefer to use an ingester which have been fully setup to use the ingest storage (e.g. fake Kafka, etc...). I think it will also simplify your setup in TestOwnedSeriesServiceWithPartitionsRing
.
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.
Maybe it's not easy to restart the ingester in the "new user trigger from WAL replay" test case, but I would give it a try anyway.
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.
Gave that a try. Not sure if it's simpler, but I've realized that I should push series via Kafka, and not Push
method.
7fb201d
to
adf2ec4
Compare
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
adf2ec4
to
3c9a4ff
Compare
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.
Thanks!
What this PR does
This PR modifies "owned series" service to make it work with partitions ring (if ingest storage is configured) as well as ingester ring.
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]
.about-versioning.md
updated with experimental features.