Transaction manager refactoring #19889

mmaslankaprv · 2024-06-19T08:57:01Z

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_cache and tx_stm_cache_manager completely.

Backports Required

Release Notes

none

mmaslankaprv · 2024-06-25T12:35:27Z

/dt

mmaslankaprv · 2024-06-25T15:29:42Z

/dt

mmaslankaprv · 2024-06-26T13:08:32Z

/dt

mmaslankaprv · 2024-06-28T13:50:55Z

/dt

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]>

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]>

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]>

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]>

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]>

mmaslankaprv · 2024-06-28T15:17:14Z

/dt

bharathv · 2024-06-29T00:18:58Z

src/v/kafka/server/errors.h

+    case cluster::tx::errc::request_rejected:
+    case cluster::tx::errc::unknown_server_error:
+        return error_code::unknown_server_error;
+        break;


nit: redundant break.

bharathv · 2024-06-29T00:21:19Z

src/v/kafka/server/errors.h

+    case cluster::tx::errc::preparing_rebalance:
+    case cluster::tx::errc::rebalance_in_progress:
+    case cluster::tx::errc::coordinator_load_in_progress:
+    case cluster::tx::errc::request_rejected:


these seem to be retriable at a high level but are mapped to USE which is terminal state and requires producer reset?

bharathv · 2024-06-29T00:22:26Z

src/v/kafka/server/errors.h

@@ -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) {


wonder if this belongs in tx_err.h? kafka/server/errors.h depending on (internal) tx_err seems a bit odd.

bharathv · 2024-06-29T00:33:27Z

tests/rptest/transactions/compaction_e2e_test.py

@@ -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


curious why?

bharathv · 2024-06-29T00:34:34Z

tests/rptest/transactions/transactions_test.py

@@ -481,7 +481,7 @@ def expired_tx_test(self):
            assert False, "tx is expected to be expired"
        except ck.cimpl.KafkaException as e:
            kafka_error = e.args[0]
-            assert kafka_error.code() == ck.cimpl.KafkaError._FENCED
+            assert kafka_error.code() == ck.cimpl.KafkaError.INVALID_TXN_STATE


is this commit supposed to come after your (deeper) changes? or is it because of changes in error code mapping commit?

bharathv

still looking at the last two (main) commits, fleshed out the remaining comments.

bharathv · 2024-06-29T01:29:09Z

src/v/cluster/tm_stm_types.cc

        return "PrepareAbort";
-    case tx_status::killed:
+    case tx_status::expired:


https://redpandadata.slack.com/archives/C01ND4SVB6Z/p1719592501245829

I don't know this mapping is 100% correct, at-least confusing users.

bharathv · 2024-06-29T01:35:34Z

src/v/cluster/tm_stm_types.h

+    preparing_abort = 3,
+    /**
+     * The same as the preparing_abort but when transaction is aborted by
+     * internals of Redpand f.e. timeout or transactional id limit overflow


typo Redpanda

bharathv · 2024-06-29T01:36:34Z

src/v/cluster/tm_stm_types.h

@@ -15,16 +15,63 @@
 namespace cluster {

 enum tx_status : int32_t {


this fsm is a great candidate for ascii art :D

bharathv · 2024-06-29T05:37:03Z

src/v/cluster/tm_stm_types.h

+    preparing_commit = 1,
+    /**
+     * Transaction is completed commit when its participants committed the
+     * transaction. This state is not replicated as committing data on


This state is not replicated

I see finish_transaction() is replicating these, did I miss something or is this comment stale?

bharathv · 2024-06-29T05:41:07Z

src/v/cluster/tm_stm_types.cc

+  , to(to) {}
+
+bool is_state_transition_valid(
+  const tx_metadata& current, tx_status requested_status) {


nit: requested_status -> target_status?

bharathv · 2024-06-29T05:48:44Z

src/v/cluster/tm_stm_types.h

+     */
+    completed_abort = 7,
+    /**
+     * Transaction is about to be removed


nit: a little more detail here may be useful, under what context is this state possible.

bharathv · 2024-06-29T05:50:03Z

src/v/cluster/tm_stm_types.cc

+    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);


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.

bharathv · 2024-06-29T05:51:56Z

src/v/cluster/tm_stm_types.cc

+          tx_status::completed_abort,
+          tx_status::completed_commit);


this transition is a bit non intuitive IMO, especially when we say empty is the begin state.

bharathv · 2024-06-29T05:53:44Z

src/v/cluster/tm_stm_types.cc

+    case expired:
+        return is_one_of(
+          current.status,
+          tx_status::empty,


while empty -> expired makes sense I wonder if that is possible. Currently auto expiration is only possible if there is a partition added (it is initiated by the rm_stm), without a partition no one triggers it?

github-actions bot added the area/redpanda label Jun 19, 2024

mmaslankaprv marked this pull request as draft June 19, 2024 09:09

mmaslankaprv force-pushed the tx-tm-stm-refector branch 3 times, most recently from 1a1c84c to 169342f Compare June 25, 2024 12:30

mmaslankaprv force-pushed the tx-tm-stm-refector branch 2 times, most recently from a99252a to cb3023c Compare June 25, 2024 15:06

mmaslankaprv force-pushed the tx-tm-stm-refector branch from cb3023c to 4a53e12 Compare June 26, 2024 12:36

mmaslankaprv force-pushed the tx-tm-stm-refector branch 2 times, most recently from a7ebc00 to 70d07d1 Compare June 28, 2024 13:43

mmaslankaprv requested review from bharathv, ztlpn and bashtanov June 28, 2024 13:53

mmaslankaprv added 10 commits June 28, 2024 14:40

group: added warn message when a producer was fenced by group update

fcb195e

Signed-off-by: Michał Maślanka <[email protected]>

m/record: fixed exhausted epoch definition

72305d8

The epoch is exhausted if it is equal to the max possible epoch - 1. Signed-off-by: Michał Maślanka <[email protected]>

k/server: unified mapping of tx error codes

abb5b57

Unified mapping of transactional error codes to kafka protocol error codes. Signed-off-by: Michał Maślanka <[email protected]>

tx_gateway: do not depend on controller

f9b8d7c

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]>

tx: fixed typo in add_partitions_tx_request type name

06e48db

Signed-off-by: Michał Maślanka <[email protected]>

tm_stm: moved variables definition to the bottom of the class

5accfe9

Signed-off-by: Michał Maślanka <[email protected]>

c/tm_stm: replaced flat hash maps with chunked hash maps

be13f46

Replaced absl flat hash maps with chunked version to prevent memory fragmentation. Signed-off-by: Michał Maślanka <[email protected]>

mmaslankaprv added 5 commits June 28, 2024 15:04

tests: limit the size of segment in compaction test

ce08ee3

Signed-off-by: Michał Maślanka <[email protected]>

tests: updated expired transaction test

a450332

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]>

c/transactions: added definition of transaction metadata fsm

6bbc3ba

Added a function validating state transition of transaction FSM. Signed-off-by: Michał Maślanka <[email protected]>

mmaslankaprv force-pushed the tx-tm-stm-refector branch from 70d07d1 to 2458b2f Compare June 28, 2024 15:11

bharathv reviewed Jun 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transaction manager refactoring #19889

Transaction manager refactoring #19889

mmaslankaprv commented Jun 19, 2024 •

edited

Loading

mmaslankaprv commented Jun 25, 2024

mmaslankaprv commented Jun 25, 2024

mmaslankaprv commented Jun 26, 2024

mmaslankaprv commented Jun 28, 2024

mmaslankaprv commented Jun 28, 2024

bharathv Jun 29, 2024

bharathv Jun 29, 2024

bharathv Jun 29, 2024

bharathv Jun 29, 2024

bharathv Jun 29, 2024

bharathv left a comment

bharathv Jun 29, 2024

bharathv Jun 29, 2024

bharathv Jun 29, 2024

bharathv Jun 29, 2024

bharathv Jun 29, 2024

bharathv Jun 29, 2024

bharathv Jun 29, 2024

bharathv Jun 29, 2024

bharathv Jun 29, 2024

		@@ -15,16 +15,63 @@
		namespace cluster {

		enum tx_status : int32_t {

Transaction manager refactoring #19889

Are you sure you want to change the base?

Transaction manager refactoring #19889

Conversation

mmaslankaprv commented Jun 19, 2024 • edited Loading

Backports Required

Release Notes

mmaslankaprv commented Jun 25, 2024

mmaslankaprv commented Jun 25, 2024

mmaslankaprv commented Jun 26, 2024

mmaslankaprv commented Jun 28, 2024

mmaslankaprv commented Jun 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bharathv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmaslankaprv commented Jun 19, 2024 •

edited

Loading