WIP: Archived LSN auto-trimming #2390
Conversation
There was a problem hiding this comment.
Gave the draft PR a quick glance. My main question would be whether we should start with the trim gap support first because some of the code adds a temporary solution for handling the absence of the trim gap support which might not be necessary if we sequence the work differently.
| assert_eq!(Lsn::INVALID, bifrost.get_trim_point(LOG_ID).await?); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| #[test(restate_core::test(start_paused = true))] | ||
| async fn do_not_trim_if_dead_nodes_present() -> anyhow::Result<()> { |
There was a problem hiding this comment.
What if a node has permanently been decommissioned?
There was a problem hiding this comment.
This must be dealt with somehow; I think this is telling us that we're missing an operational tool to explicitly remove permanently decommissioned nodes. We might also want to add a tolerance level to the auto-trim component for dead/unknown/lagging nodes.
|
|
||
| if max_archived_lsn == Lsn::INVALID { | ||
| warn!("Not trimming log '{log_id}' because no node is reporting a valid archived LSN"); | ||
| } else if min_applied_lsn < current_trim_point { |
There was a problem hiding this comment.
If the implementation works correctly, shouldn't min_applied_lsn be always smaller than the current trim point?
There was a problem hiding this comment.
Indeed, there is a mistake here, which I only fixed once I was on the airplane and couldn't push 😅 It's corrected locally but I'll park this for now.
| "Not trimming log '{log_id}' because some nodes have not applied the log up to the archived LSN" | ||
| ); | ||
| } else if max_archived_lsn >= current_trim_point { | ||
| safe_trim_points.insert(log_id, (max_archived_lsn, partition_id)); |
There was a problem hiding this comment.
Shouldn't we check that min_applied_lsn is larger than max_archived_lsn to have a safe trim point?
There was a problem hiding this comment.
As above, already fixed locally - obvious mistake which I didn't catch until I tested this for the first time.
| // todo(pavel): remove this check once we implement trim-gap support (https://github.com/restatedev/restate/issues/2247) | ||
| warn!( | ||
| %partition_id, | ||
| "Not trimming log '{log_id}' because some nodes have not applied the log up to the archived LSN" | ||
| ); |
There was a problem hiding this comment.
I am wondering whether we should implement the trim-gap support first because then we don't have to add these safety measures. Moreover, it would solve the following problem right now: What if a node is not reporting that it runs a PP because the command to start the PP is still in flight. In this case, I believe we might trim even though the PP which is about to be started has not reached the max_archived_lsn.
There was a problem hiding this comment.
This is a great observation! Makes sense, let's start with trim gap support and revive this after that's in.
There was a problem hiding this comment.
Thanks a lot, @tillrohrmann! It does make sense to me to park this and tackle trim gaps first. Let's see if I can get to it in the next couple of days - please ping me if someone else picks this up :-)
|
|
||
| if max_archived_lsn == Lsn::INVALID { | ||
| warn!("Not trimming log '{log_id}' because no node is reporting a valid archived LSN"); | ||
| } else if min_applied_lsn < current_trim_point { |
There was a problem hiding this comment.
Indeed, there is a mistake here, which I only fixed once I was on the airplane and couldn't push 😅 It's corrected locally but I'll park this for now.
| "Not trimming log '{log_id}' because some nodes have not applied the log up to the archived LSN" | ||
| ); | ||
| } else if max_archived_lsn >= current_trim_point { | ||
| safe_trim_points.insert(log_id, (max_archived_lsn, partition_id)); |
There was a problem hiding this comment.
As above, already fixed locally - obvious mistake which I didn't catch until I tested this for the first time.
pcholakov
force-pushed
the
feat/archived-lsn-log-trimming
branch
from
92bd1c5 to
7a4b2d7
Compare
Closes: #1812