CreateSnapshot RPC returns min applied LSN #2743
Conversation
This enables the restatectl tool to print the minimum included LSN when ad-hoc snapshots are created. This also enables the trim gap test to trim right up to the snapshotted LSN.
| @@ -74,7 +75,13 @@ pub struct CreateSnapshotRequest { | |||
|
|
|||
| #[derive(Debug, Clone, Serialize, Deserialize)] | |||
| pub struct CreateSnapshotResponse { | |||
| pub result: Result<SnapshotId, SnapshotError>, | |||
| pub result: Result<Snapshot, SnapshotError>, | |||
There was a problem hiding this comment.
Is this a reasonable shape for this RPC response? I made it this way when I first introduced it thinking it would be more evolvable - is that actually true? Should this just be a type alias for Result<Snapshot, SnapshotError> rather?
There was a problem hiding this comment.
Compared to the previous version, that's better, although in the future we won't allow for marshaling "Result" like this. Since it has been already like this, I don't mind the change you introduced.
There was a problem hiding this comment.
Ah, sorry about that! I remember I called attention to this during the initial PR but didn't get clarity on this.
| @@ -74,7 +75,13 @@ pub struct CreateSnapshotRequest { | |||
|
|
|||
| #[derive(Debug, Clone, Serialize, Deserialize)] | |||
| pub struct CreateSnapshotResponse { | |||
| pub result: Result<SnapshotId, SnapshotError>, | |||
| pub result: Result<Snapshot, SnapshotError>, | |||
There was a problem hiding this comment.
Compared to the previous version, that's better, although in the future we won't allow for marshaling "Result" like this. Since it has been already like this, I don't mind the change you introduced.
| @@ -133,11 +133,18 @@ async fn fast_forward_over_trim_gap() -> googletest::Result<()> { | |||
| .create_partition_snapshot(CreatePartitionSnapshotRequest { partition_id: 0 }) | |||
| .await? | |||
| .into_inner(); | |||
| info!( | |||
| "Snapshot created up to at least LSN {}", | |||
There was a problem hiding this comment.
"at least" might be misleading. Perhaps "that include LSN or higher"? Alternatively, describe it terms of what it doesn't have, i.e. "include changes up to LSN"
| "Snapshot created for partition {partition_id}: {} (LSN >= {})", | ||
| response.snapshot_id, | ||
| response.min_applied_lsn, |
There was a problem hiding this comment.
As a habbit, I wish we communicate the log-id when we talk about LSNs in general. I understand that the current equivalence of partition-id to log-id makes it redundant,, but it'll not always be like that.
There was a problem hiding this comment.
Noted! It felt a bit premature to do so here but I'll make it a point to do so in the future. I'll update the response shape to return an explicit log id here, and have the server encapsulate the mapping.
There was a problem hiding this comment.
Ugh, making the change I wanted to make - have CreatePartitionSnapshotResponse carry an explicit log id - is quite far-reaching. Let me send that as a follow-up PR.
This enables the restatectl tool to print the minimum included LSN when ad-hoc snapshots are created.
This also enables the trim gap test to trim right up to the snapshotted LSN.