Use journal table v2 as default #3921
Conversation
…ice invocation ingestion logic to write the journal v2
… 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
slinkydeveloper
changed the title
|
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
force-pushed
the
issues/3184-add-feature-flag
branch
from
257c1d9 to
60b2632
Compare
|
|
This case can happen if we got an invocation scheduled (or inboxed) before the journal v2 default feature was enabled.
|
Fixed the problem with exclusions! |
|
did some manual testing locally, primarily around scheduled invocations, first disabling then enabling, and viceversa. So far everything seems to work as expected. |
tillrohrmann
left a comment
tillrohrmann
left a comment
There was a problem hiding this comment.
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.
| // 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, | ||
| }) | ||
| } |
There was a problem hiding this comment.
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?
|
@slinkydeveloper can you resolve my comments and update this PR so that I can rebase #3953 on it? |
|
Merged via b9faafa |
2 participants
Part of #3184