Use journal table v2 as default #3921

slinkydeveloper · 2025-10-22T12:47:39Z

Part of #3184

…ice invocation ingestion logic to write the journal v2

… and then tries to read from journal table v1

slinkydeveloper · 2025-10-22T19:44:25Z

This is ready for an initial pass. The test is failing because i still didn't figure out how to do the exclusion properly 🫨 but the exclusion is legit

…tever reason the negotiated protocol when invoking will be less than 4.

…he problematic situations. Fix some incorrectly handled corner cases, such as deletion of journal when the pinned deployment is not set yet.

…tion. See the changes for more details.

slinkydeveloper · 2025-10-22T19:57:52Z

~~TODO: in the state machine init_journal, better to also init there the journal using journal v2. this sorts out the situation where users update with scheduled invocations.~~

This case can happen if we got an invocation scheduled (or inboxed) before the journal v2 default feature was enabled.

slinkydeveloper · 2025-10-23T12:00:14Z

Fixed the problem with exclusions!

slinkydeveloper · 2025-10-23T12:21:45Z

did some manual testing locally, primarily around scheduled invocations, first disabling then enabling, and viceversa. So far everything seems to work as expected.

tillrohrmann

Thanks a lot for preparing a future where we can drop the old state machine and the old invoker @slinkydeveloper. The changes look good to me. I left a few comments about adding a bit more context for identifying code paths that are legacy and will get removed. Also the usage of rstest seems not strictly needed and could allow us to reduce this dependency eventually if we don't add more tests using it.

As a side comment: The state machine has become quite hard to reason about with the new and old code paths. I am really looking forward simplifying things when we get rid of the old code paths.

tillrohrmann · 2025-11-04T16:22:18Z

crates/invoker-impl/src/error.rs

        expected: InvocationEpoch,
    },
+    #[error(
+        "error when reading the journal: expected to read {expected} entries, but read only {expected}. This indicates a bug or a storage corruption."


Suggested change

"error when reading the journal: expected to read {expected} entries, but read only {expected}. This indicates a bug or a storage corruption."

"error when reading the journal: expected to read {expected} entries, but read only {actual}. This indicates a bug or a storage corruption."

tillrohrmann · 2025-11-04T16:26:49Z

crates/invoker-impl/src/invocation_task/service_protocol_runner_v4.rs

+                            // Let's verify if we sent all the entries we promised, otherwise the stream will hang in a bad way!
+                            if sent_entries < expected_entries_count {
+                                return TerminalLoopState::Failed(InvokerError::UnexpectedEntryCount {
+                                    actual: sent_entries,
+                                    expected: expected_entries_count,
+                                })
+                            }


Is this a situation that couldn't arise before and is now possible due to the changes of the PR or is this covering a case that could have happened before as well?

tillrohrmann · 2025-11-04T16:35:34Z

crates/worker/src/partition/rpc/restart_as_new_invocation.rs

+                    Err(storage_error) => {
+                        replier.send_result(Err(PartitionProcessorRpcError::Internal(
+                            storage_error.to_string(),
+                        )));
+                        return Ok(());


A bit surprising that we reply with an error but return Ok(()). It seems that the return value of RpcHandler::handle is ignored here

restate/crates/worker/src/partition/mod.rs

Line 618 in cd0423b

let _ = rpc::RpcHandler::handle(

.

tillrohrmann · 2025-11-04T16:40:04Z

crates/worker/src/partition/rpc/restart_as_new_invocation.rs

            PatchDeploymentId::KeepPinned => {
                // Just keep current deployment, all good
-                pinned_deployment.deployment_id
+                None


Why are we no longer keeping the previous deployment id if we should keep it?

tillrohrmann · 2025-11-04T17:03:22Z

crates/worker/src/partition/state_machine/entries/notification.rs

        test_env.shutdown().await;
    }

+    #[rstest]


Instead of bringing in the rstest dependency here, we could have a run_test_notify_signal_received_before_pinned_deployment(features: EnumSet<Feature>) and then have two tests that call the method with the respective EnumSet values. This is probably not more verbose and wouldn't depend on rstest.

tillrohrmann · 2025-11-05T15:43:39Z

.github/workflows/ci.yml

+          # Needed for backward compatibility tests 1.6 -> 1.5
+          envVars: |
+            RESTATE_WORKER__INVOKER__experimental_features_allow_protocol_v6=true
+            RESTATE_EXPERIMENTAL_FEATURE__USE_JOURNAL_V2_BY_DEFAULT=true


By enabling the journal v2 usage by default, aren't we giving up forward compatibility of v1.5? Why are we testing this case?

tillrohrmann · 2025-11-05T15:48:38Z

crates/worker/src/partition/invoker_storage_reader.rs

+                    ),
+                    entries,
+                )
            } else {


This branch will no longer be necessary once we have the "eager" migration from v1 to v2 on the version barrier, right?

tillrohrmann · 2025-11-05T16:18:05Z

crates/worker/src/partition/state_machine/mod.rs

+        // 0. Prepare the journal table v2
+        if self.features.contains(Feature::UseJournalTableV2AsDefault)
+            && let PreFlightInvocationArgument::Input(PreFlightInvocationInput {
+                argument,
+                headers,
+                span_context,
+            }) = pre_flight_invocation_metadata.input
+        {
+            // In this case, we do the following:
+            // * Write the input in the journal table v2
+            // * Change pre_flight_invocation_metadata.input
+
+            // Prepare the new entry
+            let new_entry: journal_v2::Entry = InputCommand {
+                headers,
+                payload: argument,
+                name: Default::default(),
+            }
+            .into();
+            let new_raw_entry = new_entry.encode::<ServiceProtocolV4Codec>();
+
+            // Now write the entry in the new table
+            journal_table_v2::WriteJournalTable::put_journal_entry(
+                self.storage,
+                invocation_id,
+                0,
+                &StoredRawEntry::new(
+                    StoredRawEntryHeader::new(self.record_created_at),
+                    new_raw_entry,
+                ),
+                &[],
+            )?;
+
+            // Input is now a journal directly
+            pre_flight_invocation_metadata.input =
+                PreFlightInvocationArgument::Journal(PreFlightInvocationJournal {
+                    journal_metadata: JournalMetadata {
+                        length: 1,
+                        commands: 1,
+                        span_context,
+                    },
+                    pinned_deployment: None,
+                });
+        }


It took me quite a while to understand why this snippet here isn't a duplicate compared to the one in init_journal. The snippet in init_journal won't be executed because pre_flight_invocation_metadata.input is PreFlighInvocationArgument::Journal. This is quite subtle and hard to follow because one needs to trace back how the invocation_input which is being passed to init_journal_and_invoke is being constructed. Maybe add a comment to init_journal explaining a little bit the different cases, which one of them belongs to which version and which case will soon be removed with what version.

slinkydeveloper added 3 commits October 22, 2025 14:39

Starting point: introduce experimental feature and use it in the serv…

8d319fc

…ice invocation ingestion logic to write the journal v2

Allow to enable the feature flag via env, primarily for testing purpose

1d5ea2e

Invoker storage reader now tries to read first from journal table v2,…

4b81531

… and then tries to read from journal table v1

slinkydeveloper force-pushed the issues/3184-add-feature-flag branch 7 times, most recently from 443e68d to 257c1d9 Compare October 22, 2025 19:20

slinkydeveloper changed the title ~~[WIP] Use journal table v2 as default~~ Use journal table v2 as default Oct 22, 2025

slinkydeveloper requested a review from tillrohrmann October 22, 2025 19:43

slinkydeveloper marked this pull request as ready for review October 22, 2025 19:43

slinkydeveloper added 6 commits October 22, 2025 21:46

Handle the case where in storage we use journal table v2, but for wha…

c0c0f95

…tever reason the negotiated protocol when invoking will be less than 4.

Add some unit test coverage of journal table v2 feature, especially t…

99563a4

…he problematic situations. Fix some incorrectly handled corner cases, such as deletion of journal when the pinned deployment is not set yet.

Nit: rename experimental features to just features.

cd0423b

Little change to the workaround for restart as new RPC handler

2ff6495

Little thing

c2541b4

Excluded few e2e tests when running journal v2 default test configura…

60b2632

…tion. See the changes for more details.

slinkydeveloper force-pushed the issues/3184-add-feature-flag branch from 257c1d9 to 60b2632 Compare October 22, 2025 19:46

slinkydeveloper added this to the 1.6 milestone Oct 22, 2025

slinkydeveloper added 4 commits October 23, 2025 09:39

Write to journal table v2 by default also in the init_journal function.

b45f03d

This case can happen if we got an invocation scheduled (or inboxed) before the journal v2 default feature was enabled.

Test some corner case with suspension and signals

4f4528e

Little rename

b30bef6

Figured the spell for exclusions

a127b80

slinkydeveloper added the release-blocker Blocker for the next release label Oct 29, 2025

AhmedSoliman self-requested a review October 30, 2025 09:24

tillrohrmann approved these changes Nov 5, 2025

View reviewed changes

tillrohrmann mentioned this pull request Nov 5, 2025

Introduce version barrier for enabling journal v2 by default #3953

Open

	"error when reading the journal: expected to read {expected} entries, but read only {expected}. This indicates a bug or a storage corruption."
	"error when reading the journal: expected to read {expected} entries, but read only {actual}. This indicates a bug or a storage corruption."

Use journal table v2 as default #3921

Are you sure you want to change the base?

Use journal table v2 as default #3921

Uh oh!

Conversation

slinkydeveloper commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slinkydeveloper commented Oct 22, 2025

Uh oh!

slinkydeveloper commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slinkydeveloper commented Oct 23, 2025

Uh oh!

slinkydeveloper commented Oct 23, 2025

Uh oh!

tillrohrmann left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

slinkydeveloper commented Oct 22, 2025 •

edited

Loading

slinkydeveloper commented Oct 22, 2025 •

edited

Loading