Transaction manager refactoring #19889
Merged
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
mmaslankaprv
force-pushed
the
tx-tm-stm-refector
branch
3 times, most recently
from
1a1c84c
to
169342f
Compare
|
/dt |
mmaslankaprv
force-pushed
the
tx-tm-stm-refector
branch
2 times, most recently
from
a99252a
to
cb3023c
Compare
|
/dt |
mmaslankaprv
force-pushed
the
tx-tm-stm-refector
branch
from
cb3023c
to
4a53e12
Compare
|
/dt |
mmaslankaprv
force-pushed
the
tx-tm-stm-refector
branch
2 times, most recently
from
a7ebc00
to
70d07d1
Compare
|
/dt |
mmaslankaprv
force-pushed
the
tx-tm-stm-refector
branch
from
70d07d1
to
2458b2f
Compare
|
/dt |
bharathv
reviewed
Jun 29, 2024
| @@ -113,4 +114,48 @@ constexpr error_code map_topic_error_code(cluster::errc code) { | |||
| return error_code::unknown_server_error; | |||
| } | |||
|
|
|||
| constexpr error_code map_tx_errc(cluster::tx::errc ec) { | |||
There was a problem hiding this comment.
wonder if this belongs in tx_err.h? kafka/server/errors.h depending on (internal) tx_err seems a bit odd.
There was a problem hiding this comment.
i think the other way around would be incorrect as it would make cluster submodule depend on kafka submodule (it already kind of does), this is the kafka protocol implementation detail on how to map a more internal error code to user facing protocol defined errors.
bharathv
reviewed
Jun 29, 2024
| @@ -29,7 +29,7 @@ class CompactionE2EIdempotencyTest(RedpandaTest): | |||
| def __init__(self, test_context): | |||
| extra_rp_conf = {} | |||
|
|
|||
| self.segment_size = 5 * 1024 * 1024 | |||
| self.segment_size = 1 * 1024 * 1024 | |||
There was a problem hiding this comment.
this test was failing to me even before the refactoring.
bharathv
reviewed
Jun 29, 2024
bharathv
reviewed
Jun 29, 2024
| case empty: | ||
| // ready is an initial state a transaction can never go back to that | ||
| // state | ||
| return is_one_of(current.status, tx_status::empty); |
There was a problem hiding this comment.
nit: I think one confusing bit here is the state transition from to itself (current == target) is a valid transition. Is that intentional? That seems to be the case for all the states.
There was a problem hiding this comment.
yes, this is intentional for the transitions to be idempotent, i will think more about this
mmaslankaprv
force-pushed
the
tx-tm-stm-refector
branch
2 times, most recently
from
50585f4
to
20d8a4e
Compare
|
/dt |
mmaslankaprv
force-pushed
the
tx-tm-stm-refector
branch
from
20d8a4e
to
9e66e84
Compare
mmaslankaprv
force-pushed
the
tx-tm-stm-refector
branch
from
cf47946
to
9391bc3
Compare
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/51212#01909116-c9c4-409e-bebc-7c535c770d34 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/51212#01909116-c9c6-49bd-99bb-e7fadc9f303b ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/51362#0190a2ae-d798-457e-b12e-38ef9284fdb0 |
mmaslankaprv
force-pushed
the
tx-tm-stm-refector
branch
from
9391bc3
to
c2ea2eb
Compare
|
Full chaos run using the latest build is at https://buildkite.com/redpanda/vtools/builds/15441#_ |
mmaslankaprv
force-pushed
the
tx-tm-stm-refector
branch
from
c2ea2eb
to
3732a2f
Compare
Previously the test was not testing what it expected to test as the error was validated against was incorrect. The test passed as the transaction was already timeouted when group was finally rebalanced. Signed-off-by: Michał Maślanka <[email protected]>
Signed-off-by: Michał Maślanka <[email protected]>
When producers are evicted from the `rm_stm` the timer is fired based on the requested eviction timeout. Added an upper bound on the frequency of timer fires to prevent busy loops. Signed-off-by: Michał Maślanka <[email protected]>
The epoch is exhausted if it is equal to the max possible epoch - 1. Signed-off-by: Michał Maślanka <[email protected]>
Introduced an error code indicating existence of transaction concurrent to the one client is interested in. Concurrent transactions error indicates that client should retry without requesting a metadata update. Signed-off-by: Michał Maślanka <[email protected]>
Unified mapping of transactional error codes to kafka protocol error codes. Signed-off-by: Michał Maślanka <[email protected]>
Signed-off-by: Michał Maślanka <[email protected]>
Signed-off-by: Michał Maślanka <[email protected]>
Replaced absl flat hash maps with chunked version to prevent memory fragmentation. Signed-off-by: Michał Maślanka <[email protected]>
When transaction is expired on one of the partitions the coordinator is requested to send abort requests to other transactions participants. Changed the test to account for that logic instead of requesting each data partition separately to abort the transaction. Signed-off-by: Michał Maślanka <[email protected]>
mmaslankaprv
force-pushed
the
tx-tm-stm-refector
branch
from
3732a2f
to
8d93e1f
Compare
Added a function validating state transition of transaction FSM. Signed-off-by: Michał Maślanka <[email protected]>
Previously add partition to transaction and add group to transaction data batches were not persisted in transaction manager log. This made transactions implementation vulnerable to coordinator leadership changes. Changed the code to base on replicating each transaction state update. This commit includes a major refactoring of transactions subsystem including: - `tx_status` values rename - dropping the tm_stm_cache and moving the state to tm_stm - refining the logic of transaction coordinator and gateway Signed-off-by: Michał Maślanka <[email protected]>
The dependency on `cluster::controller` is not required as it is enough for the gateway to know current node id. Signed-off-by: Michał Maślanka <[email protected]>
Now the transaction state validation is more strict so the coordinator started to send more appropriate error codes. Signed-off-by: Michał Maślanka <[email protected]>
The transactional coordinator state machine is waiting for application of batches to the state when handling client requests it is critical for the application to happen timely. Previously the coordinator topic wasn't using `storage::batch_cache` which made the appended batches not immediately available to read by the state machine manager. In this case the manager waits for some time to prevent busy looping. Enabling the cache for the transaction manager topic makes the operations much faster and decreases the client latency. Signed-off-by: Michał Maślanka <[email protected]>
mmaslankaprv
force-pushed
the
tx-tm-stm-refector
branch
from
8d93e1f
to
c46f92a
Compare
benesch
added a commit
to benesch/materialize
that referenced
this pull request
Aug 14, 2024
Redpanda v24.2 includes a fix that allows our sinks to restart after a Redpanda broker failure without waiting out the full transaction timeout (often 10m+). For details about the original issue and the fix in Redpanda, see: * redpanda-data/redpanda#19889 * MaterializeInc/incidents-and-escalations#33 Fix MaterializeInc/incidents-and-escalations#33.
5 tasks
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.
Major refactor of Redpanda transaction coordinator. The refactoring involves replication of every transaction state update into coordinator partition. The changes involve fixing the transaction state machine and providing a strict and centralised definition of possible transaction state transitions. Replicating each transaction state update allowed us to drop the
tx_stm_cacheandtx_stm_cache_managercompletely.Fixes: #18184
Fixes: #18251
Backports Required
Release Notes