storage: optionally ignore timestamps in the future for retention#10028
storage: optionally ignore timestamps in the future for retention#10028jcsp merged 3 commits intoredpanda-data:devfrom
Conversation
andrwng
left a comment
andrwng
left a comment
There was a problem hiding this comment.
LGTM overall, just a question about toggling the escape hatch
src/v/storage/segment_index.h
Outdated
| _retention_timestamp = t; | ||
| } | ||
| model::timestamp retention_timestamp() const { | ||
| return _retention_timestamp.value_or(_state.max_timestamp); |
There was a problem hiding this comment.
Should we also gate this on storage_ignore_timestamps_in_future_sec, in case we switch back from ignoring timestamps and no longer want to rely on _retention_timestamp?
There was a problem hiding this comment.
I had mainly been thinking of this as a one shot thing, but you're right that it's straightforward to make it toggleable by adding a check here, so I've gone ahead and done that.
VladLazar
left a comment
VladLazar
left a comment
There was a problem hiding this comment.
Makes sense to me. We should probably write down some instructions on how to detect scenarios where bogus timestamps are blocking retention and how to use this new config to resolve the situation.
Probably something along the lines:
- Look for "segment with bogus retention timestamps" logs
- From the log lines above, determine how far into the future those timestamps are
- Set
storage_ignore_timestamps_in_future_secto something that will allow retention to proceed - Unset
storage_ignore_timestamps_in_future_secif the bogus timestamps was due to a transient client issue (perhaps the test should be extended to do this)
| # remove anything because the timestamps are too far in the future. | ||
| with self.redpanda.monitor_log(self.redpanda.nodes[0]) as mon: | ||
| # Time period much larger than what we set log_compaction_interval_ms to | ||
| sleep(10) |
There was a problem hiding this comment.
Is this sleep actually necessary? The wait until below implies that retention has kicked in.
There was a problem hiding this comment.
Mostly not, but I wanted to have a better chance of the housekeeping having entirely completed, rather than proceeding as soon as it has seen one segment that emits the bogus timestamp message.
`segment_index` now has a retention_timestamp overlap that overrides use of the index's max_timestamp as the timestamp for retention purposes. This override is set in disk_log_impl::retention_adjust_timestamps if the storage_ignore_timestamps_in_future_sec configuration is enabled and the segment's max timestamp is out of bounds. There is no change to behavior by default: this only kicks in if the new configuration property has been set. Fixes redpanda-data#9820
jcsp
force-pushed
the
issue-9820-timestamp-filter
branch
from
b5eb0ea to
ba1b0ad
Compare
|
/backport v23.1.x |
|
/backport v22.3.x |
|
Failed to run cherry-pick command. I executed the below command: |
|
Failed to run cherry-pick command. I executed the below command: |
4 participants
This is an off-by-default behavior to enable systems where a user has sent in a dramatically wrong timestamp to ask Redpanda to ignore timestamps in the future, and do its best to infer an alternative timestamp for use in retention.
Fixes #9820
Backports Required
Release Notes
Improvements
storage_ignore_timestamps_in_future_seccluster configuration property (default null). If set to non-null, then timestamps more than this many seconds in the future will be ignored by Redpanda when considering whether a segment is old enough to garbage collect.