proposal: Remote Write: Restart from segment-based savepoint#72
proposal: Remote Write: Restart from segment-based savepoint#72kgeckhart wants to merge 6 commits into
Conversation
Signed-off-by: Kyle Eckhart <kgeckhart@users.noreply.github.com>
Signed-off-by: Kyle Eckhart <kgeckhart@users.noreply.github.com>
Signed-off-by: Kyle Eckhart <kgeckhart@users.noreply.github.com>
…heckpoint Signed-off-by: Kyle Eckhart <kgeckhart@users.noreply.github.com>
96370f0 to
cdc82e6
Compare
bwplotka
left a comment
There was a problem hiding this comment.
Nice!
Thanks, this is super needed. Added some questions, but we need to play with a different strategies here. WAL segments based is a cool idea!
I'd recommend we stop talking about exact code components, it might add too much detail to the discussion (subjective opinion).
| ### Pitfalls of the current solution | ||
|
|
||
| As mentioned in the why, this behavior is often confusing to users who know a WAL is in use but still finds they have missing data on restart. |
There was a problem hiding this comment.
| ### Pitfalls of the current solution | |
| As mentioned in the why, this behavior is often confusing to users who know a WAL is in use but still finds they have missing data on restart. |
Probably dup?
| 1. Support resuming from a savepoint for each configured `remote_write` destination. | ||
| 2. Taking a savepoint for a remote_write destination should not incur significant overhead. | ||
| 3. Changing the `queue_configuration` for a `remote_write` destination should not result in a new savepoint entry. | ||
| * The `queue_configuration` includes fields like min/max shards and other performance tuning parameter.s |
There was a problem hiding this comment.
| * The `queue_configuration` includes fields like min/max shards and other performance tuning parameter.s | |
| * The `queue_configuration` includes fields like min/max shards and other performance tuning parameters. |
|
|
||
| ## Goals | ||
|
|
||
| 1. Support resuming from a savepoint for each configured `remote_write` destination. |
There was a problem hiding this comment.
Some questions:
- Do you propose it to be by default or optional?
- What to do for users who prefer fresh data over persisting old data?
- What to do for users who want persistence, but WAL grown so much they will 100% fail to catch up, so it's better to drop?
There was a problem hiding this comment.
- Do you propose it to be by default or optional?
Yes at some point in the future I think this should become the default option but I think we'll need to evolve some default config options (and potentially introduce new ones) to make it a safe transition.
- What to do for users who prefer fresh data over persisting old data?
- What to do for users who want persistence, but WAL grown so much they will 100% fail to catch up, so it's better to drop?
Thinking about this in terms of what users can do today to prevent these things. A user can use remote_write.queue_config.sample_age_limit to try to keep 2 in check, I think it's the same answer for 3. Today you cannot modify this setting "online", changing it causes us to dump the WAL so I think the answer mostly ends up being "restart it".
After we have a segment based savepoint, I think this setting is still the most valuable tool to prevent unsustainable WAL growth and manage the tradeoff between freshness and completeness of data. I suggested that enabling savepoint should come with adding a default value of 2 hours. Couple this with the adjustment to ensure queue config won't trigger a replay a user can drop this value lower to catch-up and put it back. WDYT?
|
|
||
| 1. Support resuming from a savepoint for each configured `remote_write` destination. | ||
| 2. Taking a savepoint for a remote_write destination should not incur significant overhead. | ||
| 3. Changing the `queue_configuration` for a `remote_write` destination should not result in a new savepoint entry. |
There was a problem hiding this comment.
Do you mean?
| 3. Changing the `queue_configuration` for a `remote_write` destination should not result in a new savepoint entry. | |
| 3. Changing the `queue_configuration` for a `remote_write` destination should not result in losing a savepoint entry. |
|
|
||
| ## Alternatives | ||
|
|
||
| 1. `remote.QueueManager` should own syncing its own savepoint (most early implementations took this approach). |
There was a problem hiding this comment.
I am lost here, should we avoid diving into too much of code architecture in this proposal?
e.g remote.WriteStorage sounds fancy, but it's a literally trivial HTTP client for remote write AFAIK. If we do save point there or on caller side.. it's fine to decide during implementation.
Eventually if feels this style might hard to review by maintainers who didn't look on current Prometheus RW sending code recently. Brining it to high level design (shard, queues, WAL watching) might give more chances someone else will help in review (:
| 1. `remote.QueueManager` should own syncing its own savepoint (most early implementations took this approach). | ||
| * `remote.QueueManager` already has a lot of responsibilities and will take on more for at-least-once. | ||
| * `remote.WriteStorage` has reasonable hook points to run this logic without adding a lot more complexity. | ||
| 2. The savepoint should be synchronously updated when segments change. |
There was a problem hiding this comment.
| 2. The savepoint should be synchronously updated when segments change. | |
| 2. The savepoint should be synchronously updated when segments change during WAL watching. |
| * `remote.QueueManager` already has a lot of responsibilities and will take on more for at-least-once. | ||
| * `remote.WriteStorage` has reasonable hook points to run this logic without adding a lot more complexity. | ||
| 2. The savepoint should be synchronously updated when segments change. | ||
| * Introducing a bit of time between knowing that a segment changed to persisting it gives us more time to fully deliver the batch before we persist the change. |
There was a problem hiding this comment.
Can we split into Pros and Cons?
The pro is simpler implementation. Plus given queues have limit (they can be full) WAL watching tracking could be good enough persistence. Not saying this is the best option - but something to consider.
|
|
||
| ### Code flow | ||
|
|
||
| 1. Adding another configurable timer to [`remote.WriteStorage.run()`](https://github.com/prometheus/prometheus/blob/f50ff0a40ad4ef24d9bb8e81a6546c8c994a924a/storage/remote/write.go#L114-L125) periodically persisting the current segments for each queue. |
There was a problem hiding this comment.
Hmmmm, if we do it periodically anyway then relying only on WAL watching tracking might be good enough? 🤔
There was a problem hiding this comment.
Also... why periodic, isn't segment rarely rotated/finished?
There was a problem hiding this comment.
Hmmmm, if we do it periodically anyway then relying only on WAL watching tracking might be good enough? 🤔
That was my hypothesis when looking at alternatives which included a synchronous commit,
- If we assume a 15 second queue delay then syncing the savepoint every 30 seconds gives a lot of room for the segment to be fully processed before being committed.
…tion proposal, expand more on at-least-once, alternatives pros/cons Signed-off-by: Kyle Eckhart <kgeckhart@users.noreply.github.com>
bwplotka
left a comment
There was a problem hiding this comment.
LGTM, one question, but otherwise I would love to unblock this work - I hope it brings refresh to our codebase on the way for queue manager.
| If a config is reloaded or prometheus/agent is restarted before flushing pending samples we will skip those samples. | ||
| Given we have a persistent WAL this behavior is unexpected by users and can cause a lot of confusion. | ||
|
|
||
| ## Goals |
There was a problem hiding this comment.
Is the goal to make sure we also always support best-effort mode for lightweight setup that prefers lower latency and HW use?
There was a problem hiding this comment.
Ahh good point, I had actually assumed we would always want this behavior at some point in the future. As we introduce it, it will absolutely need to be opt-in but I imagine if we could make the breaking change we would want it on by default with an opt-out? Is that your perspective as well?
I am proposing a solution to allow remote write to restart from a savepoint (avoiding the use of checkpoint as there's already a checkpoint involved in remote write which does not help). This is a long standing issue that has seen a few attempts with none quite being accepted. It includes starting with a more basic solution which can be iterated to eventually get to at-least-once delivery.
Related to: prometheus/prometheus#8809