Add PartitionInstanceLifecycler #483
Conversation
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>
pracucci
requested review from
pstibrany and
dimitarvdimitrov
and removed request for
pstibrany
| actorChan chan func() | ||
|
|
||
| // Whether the partitions should be created on startup if it doesn't exist yet. | ||
| createPartitionOnStartup *atomic.Bool |
There was a problem hiding this comment.
nit: Why does this need to be atomic.Bool? I would expect that this flag can only be set before the service is started, and don't expect this to be changed concurrently. (Can this flag be part of config?)
There was a problem hiding this comment.
Can this flag be part of config?
Following current mimir logic, this flag is currently set after the lifecycler is created (and before it's started). That's why I've created a setter.
Why does this need to be atomic.Bool?
Not being part of config, I think we should make it safe even if it's not expected to be changed after lifecycler is started.
There was a problem hiding this comment.
After seeing Mimir PR, I understand this choice, but I still think making this part of config would make it clear that this isn't settable after creation of lifecycler.
|
|
||
| // CreatePartitionOnStartup returns whether the lifecycle creates the partition on startup | ||
| // if it doesn't exist. | ||
| func (l *PartitionInstanceLifecycler) CreatePartitionOnStartup() bool { |
There was a problem hiding this comment.
Curious why do we need this "getter", if the flag can only be set before starting lifecycler.
There was a problem hiding this comment.
There's just a getter for any setter. I don't see it a big concern.
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
…here was a running error 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>
|
Thanks @pstibrany for your review. I've either replied or addressed comments. Could you take another look, please? |
What this PR does:
This is another PR to add another piece of the experimental partitions ring support we're working on. In this PR I'm adding
ring.PartitionInstanceLifecyclerwhich is similar toring.Lifecycler/ring.BasicLifecyclerbut for partitions ring.Contrary to
ring.BasicLifecycler,ring.PartitionInstanceLifecyclerdoesn't support hooks: reason is not that hooks are a bad thing and shouldn't be supported (actually it turned out to be a flexible design) but it's just because we don't need hooks right now, so I preferred to keep code simpler and focused on the actual use case we have.Which issue(s) this PR fixes:
N/A
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]