Move ring operations to packages where they are used. #3675
Conversation
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
pkg/ring/ring.go
Outdated
| Ruler | ||
| // Operation describes which instances can be included in the replica set, based on their state. | ||
| type Operation interface { | ||
| // ShouldExtendReplicaSet returns true if given a state of instance that's going to be |
There was a problem hiding this comment.
| // ShouldExtendReplicaSet returns true if given a state of instance that's going to be | |
| // ShouldExtendReplicaSet returns true if, given a state of instance that's going to be |
There was a problem hiding this comment.
I don't think that's correct.
There was a problem hiding this comment.
Fair enough. I think the current comment is hard to read because of the comma at added to the replica set, the replica set size. I thought adding the , suggested above could fix it.
pkg/ring/ring.go
Outdated
| Write Operation = NewOpWithReplicaSetExtension(func(s IngesterState) bool { | ||
| // We do not want to Write to Ingesters that are not ACTIVE, but we do want | ||
| // to write the extra replica somewhere. So we increase the size of the set | ||
| // of replicas for the key. This means we have to also increase the | ||
| // size of the replica set for read, but we can read from Leaving ingesters, | ||
| // so don't skip it in this case. | ||
| // NB dead ingester will be filtered later by defaultReplicationStrategy.Filter(). | ||
| return s != ACTIVE | ||
| }, ACTIVE) | ||
|
|
||
| // WriteNoExtend is like Write, but with no replicaset extension. | ||
| WriteNoExtend Operation = NewOp(ACTIVE) | ||
|
|
||
| Read Operation = NewOpWithReplicaSetExtension(func(s IngesterState) bool { | ||
| return s != ACTIVE && s != LEAVING | ||
| }, ACTIVE, LEAVING, PENDING) |
There was a problem hiding this comment.
These should actually be moved to distributor package.
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Using i.e. tiny slowdown. With some changes to |
By removing the interface, I can get it down to: ie. 11% speed improvement compared to master. |
If we don't need the interface, I would do it. But why is faster than |
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
I've pushed the change. I haven't done any analysis except running benchmark, but I think current implementation removes some interface function calls (replication strategy for extending replica set), and switch and if-condition compared to master. Furthermore, by not using the |
pkg/ring/ring.go
Outdated
| for _, s := range states { | ||
| is[s] = true | ||
| st |= (1 << s) |
What this PR does: This PR refactors
ring.Operationto interface, which allows us to define new ring "operations" in packages where they are used. "Extension of replica set" logic has been moved from "replication strategy" to operation itself. "Replication strategy" is now only filtering unwanted instances.Inspired by another operation being added in #3664.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]