tx/rm_stm: remove mem_state #18076
Merged
tx/rm_stm: remove mem_state #18076
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
the method factors out logic to update transactional state, no logic changes.
|
/dt |
|
/dt |
|
/dt |
|
/ci-repeat 3 |
'estimated' was used as an estimate for the first data batch offset (so LSO cannot exceed it) while replication is in progress. This is the last in memory state that is pending for cleanup. This commit redefines the transacation boundary to begin from fence batch rather the first data batch which allows us to get rid of this additional state which lays the foundation for producer state move.
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/48318#018f193e-897e-404f-970c-ce4a80c92930 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/48346#018f1b8e-c5fb-41ff-826a-7e4e98dc82e0 |
| @@ -561,6 +541,8 @@ class rm_stm final : public raft::persisted_stm<> { | |||
| ss::gate _gate; | |||
| // Highest producer ID applied to this stm. | |||
| model::producer_id _highest_producer_id; | |||
| // for monotonicity of computed LSO. | |||
| model::offset _last_known_lso{-1}; | |||
There was a problem hiding this comment.
nit: do not need to be initialized, it will be default initialized
There was a problem hiding this comment.
made a small change in sync() related to this.. retained this to be -1 to be compatible with previous code.
mmaslankaprv
approved these changes
Apr 29, 2024
bharathv
added a commit
to bharathv/redpanda
that referenced
this pull request
Apr 30, 2024
Chaos logs showed that there can be duplicate (back to back) begin_tx requests due to some certain client quirks beyond our control. We had some checks in place to ignore them but they were invalidated by redpanda-data#18076 which redefined the transaction boundries. After redpanda-data#18076 an entry in the ongoing_map does not mean the data has been replicated as a part of the transaction. This PR adjusts the checks accordingly. Note: This code is going away soon, so we will have better API in the producer_state to make the code more easily understandable. This PR is to make chaos happy until then.
7 tasks
bharathv
added a commit
to bharathv/redpanda
that referenced
this pull request
May 3, 2024
In earlier versions we were not considering transactions without data batches as candidates for expiry, thats not the case any more with redpanda-data#18076, adding a regression test to future proof.
This was referenced May 3, 2024
bharathv
added a commit
to bharathv/redpanda
that referenced
this pull request
May 8, 2024
In earlier versions we were not considering transactions without data batches as candidates for expiry, thats not the case any more with redpanda-data#18076, adding a regression test to future proof.
Merged
7 tasks
Lazin
pushed a commit
to Lazin/redpanda
that referenced
this pull request
Jun 1, 2024
In earlier versions we were not considering transactions without data batches as candidates for expiry, thats not the case any more with redpanda-data#18076, adding a regression test to future proof.
Lazin
pushed a commit
to Lazin/redpanda
that referenced
this pull request
Jun 12, 2024
In earlier versions we were not considering transactions without data batches as candidates for expiry, thats not the case any more with redpanda-data#18076, adding a regression test to future proof.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Another step towards consolidated state management.
This PR removes
mem_statestruct from rm_stm entirely. Main change in this PR is to get rid of mem_state::estimated map that is supposed to hold the estimated offset of the first data batch in a transaction while its replication is in progress. This estimate is in place so that any racy LSO computation on the partition does not exceed the (future) replicated offset. We remove that by redefining the transaction boundary to include the begin/fence batch in the transaction. first data batch is guaranteed tohappen afterthe fence batch, so clamping the LSO at the fence batch by redefining the transaction boundaries ensures that in replication batches are not accounted for LSO.The advantage of removing mem state is that the entire transactional state left in the stm is replayed from the log, so it is easy it to port to the new consolidated producer state definitions.
Backports Required
Release Notes