Rename Persisted LSN to Durable LSN by pcholakov · Pull Request #3277 · restatedev/restate

pcholakov · 2025-05-15T15:19:05Z

This change renames "persisted LSN" to "durable LSN" to reflect partition data which has been durably committed to local disk by a node.

This is a breaking change for the following metrics but I feel this is justified; happy to undo if there are any concerns:

restate.partition.last_applied_lsn -> restate.partition.applied_lsn
restate.partition.applied_lsn_lag -> restate.partition.applied_lsn_lag
restate.partition.last_persisted_lsn -> restate.partition.durable_lsn

Additionally:

Updates restatectl column display to "Durable LSN"
Keeps the NodeStateResponse RPC message (response to GetNodeState, used by Custer Controller) using the old naming as we still have a FlexBuffers-based WireEncode fallback implementation for these; this will be safe to rename when we drop V1 networking support
Adds a new durable_log_lsn column to the partition_state view, mirroring persisted_log_lsn (which will be separately documented as deprecated)
other purely internal usages

% ./target/debug/restatectl sql 'select partition_id, effective_mode, applied_log_lsn, persisted_log_lsn, durable_log_lsn from partition_state'
 PARTITION_ID  EFFECTIVE_MODE  APPLIED_LOG_LSN  PERSISTED_LOG_LSN  DURABLE_LOG_LSN
 0             leader          53330            50378              50378
 1             leader          52970            50379              50379
 2             leader          52624            50360              50360
 3             leader          53013            50366              50366
 4             leader          51890            50366              50366
 5             leader          53817            50361              50361
 6             leader          53013            50361              50361
 7             leader          53114            50374              50374
 8             leader          53348            50361              50361
 9             leader          53336            50362              50362
 10            leader          53421            50363              50363
 11            leader          53320            50362              50362
 12            leader          54089            50367              50367
 13            leader          55178            50366              50366
 14            leader          52526            50361              50361
 15            leader          53079            50362              50362
 16            leader          54472            50378              50378
 17            leader          53002            50378              50378
 18            leader          52672            50361              50361
 19            leader          53361            50373              50373
 20            leader          53026            50360              50360
 21            leader          53078            50381              50381
 22            leader          53895            50367              50367
 23            leader          53849            50364              50364

24 rows. Query took 10.639125ms

% ./target/debug/restatectl sql 'describe partition_state'
 COLUMN_NAME             DATA_TYPE                               IS_NULLABLE
 partition_id            UInt32                                  YES
 plain_node_id           Utf8                                    YES
 gen_node_id             Utf8                                    YES
 target_mode             Utf8                                    YES
 effective_mode          Utf8                                    YES
 updated_at              Timestamp(Millisecond, Some("+00:00"))  YES
 leader_epoch            UInt64                                  YES
 leader                  Utf8                                    YES
 applied_log_lsn         UInt64                                  YES
 last_record_applied_at  Timestamp(Millisecond, Some("+00:00"))  YES
 skipped_records         UInt64                                  YES
 replay_status           Utf8                                    YES
 persisted_log_lsn       UInt64                                  YES
 durable_log_lsn         UInt64                                  YES
 archived_log_lsn        UInt64                                  YES
 target_tail_lsn         UInt64                                  YES

16 rows. Query took 6.38025ms

Fixes: #3252

github-actions · 2025-05-15T15:44:57Z

Test Results

7 files ±0 7 suites ±0 5m 0s ⏱️ +19s
54 tests ±0 53 ✅ ±0 1 💤 ±0 0 ❌ ±0
223 runs ±0 220 ✅ ±0 3 💤 ±0 0 ❌ ±0

Results for commit f1b5d9d. ± Comparison against base commit 63496a2.

♻️ This comment has been updated with latest results.

AhmedSoliman · 2025-05-16T14:55:08Z

crates/types/src/cluster/cluster_state.rs

+    pub durable_lsn: Option<Lsn>,
    #[bilrost(11)]
-    pub last_archived_log_lsn: Option<Lsn>,
+    pub archived_lsn: Option<Lsn>,


I suspect those renames might cause deserialization errors for V1 protocol. @pcholakov did you validate backward compatibility?

I did test a mixed cluster running main and my own changes (only after your fix for gossip went in), and yes, it does work. I believe we never serialize this struct using FlexBuffers or JSON, where this could be a problem. I'd like to do some more though, I've just confirmed that nodes form a cluster, and that all nodes start up partitions - and that all nodes can see the applied + durable LSNs from their peers.

Let's Remove Serialize/Deseralize and see what breaks then.

Silly me! This is used in NodeStateResponse, which is sent in response to Cluster Controller's GetNodeState. My testing was only using restatectl to talk to servers but a V1 admin node does not see these renamed attributes properly. Reverted!

muhamadazmy · 2025-05-23T08:58:14Z

crates/storage-query-datafusion/src/partition_state/schema.rs

-        /// Last persisted log LSN
-        persisted_log_lsn: DataType::UInt64,
+        /// Durable log LSN
+        durable_log_lsn: DataType::UInt64,


This is a breaking change. This will change the sql schema

Maybe check with @igalshilman what is the best approach to update this. Do we keep both names and mark the old name as deprecated, and keep it in code for couple of versions before removing ?

@muhamadazmy we try not to do that, but we also have a versioning mechanism.
Both the CLI and the restate admin api, decelerate versions. CLI via the agent / server strings.
It happens here in the CLI https://github.com/restatedev/restate/blob/main/cli/src/clients/admin_client.rs#L150
AFIK @jackkleeman introduced this mechanism in both ends.

I should have been more careful with this; I knew I am renaming the column in the view but switched it anyway. You're right that we should keep this as-is - reverted!

PS. I believe the versioning mechanism is purely about query data encoding, rather than the schema itself. At some point we might want to think about how we version the schema too, but I don't want to open that right now.

jackkleeman · 2025-05-23T16:05:56Z

btw, i noticed there are still a few config flags related to the old persisted lsn watchdog? like the interval

pcholakov · 2025-05-26T17:41:00Z

btw, i noticed there are still a few config flags related to the old persisted lsn watchdog? like the interval

@jackkleeman I've deprecated these in #3278 - one could argue we could just as well drop them, but I figured let's give folks a heads-up :-)

AhmedSoliman · 2025-05-29T11:10:46Z

btw, i noticed there are still a few config flags related to the old persisted lsn watchdog? like the interval

I'm not sure if this is necessary. I'd remove them from new config dumps and only keep them in shadow to give a notice.

Additionally, can you send a PR to update the relevant documentation to align with these changes please?

AhmedSoliman · 2025-05-29T11:12:21Z

crates/storage-query-datafusion/src/partition_state/schema.rs

+        /// Durable log LSN (previously, persisted log LSN)
        persisted_log_lsn: DataType::UInt64,


Should we double-write to the new name so we can remove the old one eventually?

Done, great suggestion!

muhamadazmy

Thank you @pcholakov for this PR. I agree with Ahmed that we probably should double-write the new name in the schema and update the docs of the older one as deprecated so we can drop it in the future. Other than that, it's good to go

pcholakov · 2025-05-30T20:57:21Z

Thank you @pcholakov for this PR. I agree with Ahmed that we probably should double-write the new name in the schema and update the docs of the older one as deprecated so we can drop it in the future. Other than that, it's good to go

Done! Thanks for the suggestion, I'll update the docs too 😃

pcholakov · 2025-05-31T03:53:54Z

btw, i noticed there are still a few config flags related to the old persisted lsn watchdog? like the interval

I'm not sure if this is necessary. I'd remove them from new config dumps and only keep them in shadow to give a notice.

Already done in #3278!

pcholakov requested a review from muhamadazmy May 15, 2025 15:19

pcholakov marked this pull request as ready for review May 15, 2025 15:22

AhmedSoliman reviewed May 16, 2025

View reviewed changes

muhamadazmy requested changes May 23, 2025

View reviewed changes

pcholakov force-pushed the push-ovqvqktzzuvs branch from c0af645 to f25d18d Compare May 26, 2025 17:02

pcholakov force-pushed the push-ovqvqktzzuvs branch from f25d18d to 217d8bd Compare May 26, 2025 18:09

pcholakov requested review from AhmedSoliman and muhamadazmy May 26, 2025 18:10

AhmedSoliman reviewed May 29, 2025

View reviewed changes

muhamadazmy approved these changes May 30, 2025

View reviewed changes

pcholakov closed this May 30, 2025

pcholakov deleted the push-ovqvqktzzuvs branch May 30, 2025 15:33

github-actions bot locked and limited conversation to collaborators May 30, 2025

pcholakov restored the push-ovqvqktzzuvs branch May 30, 2025 15:33

pcholakov reopened this May 30, 2025

restatedev unlocked this conversation May 30, 2025

Rename Persisted LSN to Durable LSN

f1b5d9d

pcholakov force-pushed the push-ovqvqktzzuvs branch from 99aecea to f1b5d9d Compare May 30, 2025 20:53

pcholakov merged commit 3c0192b into main May 31, 2025
26 checks passed

pcholakov deleted the push-ovqvqktzzuvs branch May 31, 2025 03:55

github-actions bot locked and limited conversation to collaborators May 31, 2025

		/// Durable log LSN (previously, persisted log LSN)
		persisted_log_lsn: DataType::UInt64,

Conversation

pcholakov commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackkleeman commented May 23, 2025

Uh oh!

pcholakov commented May 26, 2025

Uh oh!

AhmedSoliman commented May 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muhamadazmy left a comment

Choose a reason for hiding this comment

Uh oh!

pcholakov commented May 30, 2025

Uh oh!

pcholakov commented May 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pcholakov commented May 15, 2025 •

edited

Loading

github-actions bot commented May 15, 2025 •

edited

Loading