group/transactions: compaction fixes #19931

bharathv · 2024-06-20T06:30:46Z

Fixes two main issues related to compaction of group transactions:

There was no enforcement of max_collectible_offset, so open transactions were candidates for compaction
tx/compaction: don't deduplicate control batches #16295 disabled compaction of all control batches (all transactional batches in group topic are control batches) effectively disabling compaction of the topic.

These issues are addressed by adding a new state machine that enforces max_collectible_offset and then allowing compaction of control batch types from group transactions.

Backports Required

Release Notes

Bug Fixes

Fixes compaction related issues of transactional data in groups topic. This prevents a case where groups topic was growing unbounded due to ineffective compaction.

bharathv · 2024-06-20T06:31:34Z

/dt

vbotbuildovich · 2024-06-20T08:58:53Z

new failures in https://buildkite.com/redpanda/redpanda/builds/50483#01903499-4b14-4281-8fe3-3ff604ca2c45:

"rptest.tests.scaling_up_test.ScalingUpTest.test_moves_with_local_retention.use_topic_property=True"

new failures in https://buildkite.com/redpanda/redpanda/builds/50483#01903499-4b16-46f1-8744-bc44f9f0834e:

"rptest.transactions.compaction_e2e_test.CompactionE2EIdempotencyTest.test_basic_compaction.initial_cleanup_policy=delete.workload=Workload.TX_UNIQUE_KEYS"
"rptest.transactions.transactions_test.TxUpgradeTest.upgrade_does_not_change_tx_coordinator_assignment_test"

new failures in https://buildkite.com/redpanda/redpanda/builds/50483#01903499-4b18-47bc-b679-07b621e5740a:

"rptest.transactions.compaction_e2e_test.CompactionE2EIdempotencyTest.test_basic_compaction.initial_cleanup_policy=compact.workload=Workload.TX_UNIQUE_KEYS"

new failures in https://buildkite.com/redpanda/redpanda/builds/50498#01903792-80c0-4daa-944f-8c8e5e87664a:

"rptest.transactions.compaction_e2e_test.CompactionE2EIdempotencyTest.test_basic_compaction.initial_cleanup_policy=delete.workload=Workload.TX_UNIQUE_KEYS"
"rptest.transactions.compaction_e2e_test.CompactionE2EIdempotencyTest.test_basic_compaction.initial_cleanup_policy=compact.workload=Workload.TX"

new failures in https://buildkite.com/redpanda/redpanda/builds/50498#01903792-80c2-4d61-a546-f2c6ec6bc20b:

"rptest.transactions.compaction_e2e_test.CompactionE2EIdempotencyTest.test_basic_compaction.initial_cleanup_policy=compact.workload=Workload.TX_UNIQUE_KEYS"

new failures in https://buildkite.com/redpanda/redpanda/builds/50509#01903998-bf65-426e-b1a1-611a9db57536:

"rptest.transactions.compaction_e2e_test.CompactionE2EIdempotencyTest.test_basic_compaction.initial_cleanup_policy=delete.workload=Workload.TX_UNIQUE_KEYS"

new failures in https://buildkite.com/redpanda/redpanda/builds/50509#01903998-bf67-47c0-8473-c9523c31fcdf:

"rptest.transactions.compaction_e2e_test.CompactionE2EIdempotencyTest.test_basic_compaction.initial_cleanup_policy=compact.workload=Workload.TX_UNIQUE_KEYS"
"rptest.tests.raft_availability_test.RaftAvailabilityTest.test_leadership_transfer"

bharathv · 2024-06-20T20:24:53Z

/dt

vbotbuildovich · 2024-06-20T22:45:33Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/50498#01903790-d830-4921-8d52-a33e3366e545

bharathv · 2024-06-21T05:51:47Z

/dt

bharathv · 2024-06-21T21:45:23Z

/dt

bharathv · 2024-06-22T17:28:37Z

/ci-repeat 3

src/v/kafka/server/group_tx_tracker_stm.h

src/v/kafka/server/group_data_parser.h

mmaslankaprv · 2024-06-25T08:45:26Z

src/v/kafka/server/group_tx_tracker_stm.h

+            return std::tie(begin_offsets, producer_to_begin);
+        }
+    };
+    using all_txs_t = absl::btree_map<kafka::group_id, per_group_state>;


any reason not to use chunked_hash_map ?

chm is not copyable because of the backing frag_vector, the code to snapshot required iteration, given this scale is very small here, used absl variants.

src/v/kafka/server/group_tx_tracker_stm.cc

mmaslankaprv · 2024-06-25T08:49:21Z

src/v/kafka/server/group_data_parser.h

+template<class Base>
+class group_data_parser {
+protected:
+    ss::future<> parse(model::record_batch b) {


it looks to me that we do not have to return a future from all the handle methods ?

its needed for handle_raft_data but not for the rest of them, I just kept it consistent as a 'future' proof incase some 'future' code wants to do something interesting than adding to some in memory state.

bharathv · 2024-06-26T15:42:58Z

Last force push is a rebase to fix conflicts from recent group changes.

src/v/kafka/server/group_manager.h

mmaslankaprv · 2024-06-27T05:46:43Z

src/v/kafka/server/group_tx_tracker_stm.cc

+
+bool group_tx_tracker_stm_factory::is_applicable_for(
+  const storage::ntp_config& config) const {
+    auto nt = model::topic_namespace{config.ntp().ns, config.ntp().tp.topic};


here we copy topic and namespace for each single batch, maybe we can directly compare topic and namespace or use model::topic_namespace_view

ztlpn · 2024-06-27T13:07:52Z

src/v/kafka/server/group_data_parser.h

+public:
+    group_data_parser() {
+        static_assert(
+          GroupDataParserBase<Base>,


can't we just write template<GroupDataParserBase Base> instead of a static assert?

The concept is called when the template is initialized and by then non virtual members are not initialized, so the type is really incomplete at that point, so the compiler complains. By the time constructor runs that is not the case, hence the static assert.

ztlpn · 2024-06-27T14:40:12Z

src/v/kafka/server/group.h

@@ -884,6 +884,11 @@ class group final : public ss::enable_lw_shared_from_this<group> {
    std::vector<model::topic_partition>
    get_expired_offsets(std::chrono::seconds retention_period);

+    bool use_new_fence_batch_type() const {


nit: I'm always suspicious of using "new" in naming anything because after a year this thing is not so new anymore so it is less clear what "new" is referring to :)

renamed and clarified in a comment.

ztlpn · 2024-06-27T14:48:27Z

src/v/features/feature_table.h

@@ -77,6 +77,7 @@ enum class feature : std::uint64_t {
    cluster_topic_manifest_format_v2 = 1ULL << 45U,
    node_local_core_assignment = 1ULL << 46U,
    unified_tx_state = 1ULL << 47U,
+    group_fence_batch_type_switch = 1ULL << 48U,


nit: a bit unclear to me what "type switch" here means? I guess the incompatible change here is that we are using a new batch type, and it would be nice to consistently use a single name in all the places that are referring to it.

ztlpn · 2024-06-27T14:59:43Z

src/v/kafka/server/group_tx_tracker_stm.cc

+
+model::offset group_tx_tracker_stm::max_collectible_offset() {
+    auto result = last_applied_offset();
+    for (const auto& [_, group_state] : _all_txs) {


hmm I wonder what the scale here is? Maybe we should just recalculate it once in maybe_end_tx?

few 100s to max 1000 at the higher end I can think of. Also do note this inflight state, that is groups with open transaction (not historical groups in the log), so very unlikely this is in double digits IMO. Adding to that this is only called from compaction patch attempting to compute the compaction boundary, which is not a hot path.

ok, sounds good

andrwng

Generally the compaction stuff looks good. I don't fully understand the nuances of the new batch type, so deferring to others there, but IIUC we are free to remove the new group tx fences, right?

andrwng · 2024-06-27T21:38:35Z

src/v/model/record_batch_types.h

@@ -50,6 +50,7 @@ enum class record_batch_type : int8_t {
    = 29, // place holder for last batch in a segment that was aborted
    role_management_cmd = 30, // role management command
    client_quota = 31,        // client quota command
+    group_fence_tx = 32,      // fence batch in group transactions


Should this go into offset_translator_batch_types? Is the offsets topic consumable with Kafka offsets?

This is in the consumer offsets topic which doesn't have kafka based consumption (non data partition)

andrwng · 2024-06-27T21:54:03Z

src/v/kafka/server/tests/group_tx_compaction_test.cc

+    // Wait until all segments are compacted and only two remain
+    co_await log->flush();
+    co_await log->force_roll(ss::default_priority_class());
+    co_await tests::cooperative_spin_wait_with_timeout(30s, [&]() {


nit: might be able to use RPTEST_REQUIRE_EVENTUALLY_CORO here?

ya I was trying to remember this name but couldn't, thanks, will switch.

andrwng · 2024-06-27T21:58:49Z

src/v/storage/compaction_reducers.cc

@@ -367,105 +367,50 @@ ss::future<> index_rebuilder_reducer::do_index(model::record_batch&& b) {
    });
 }

-void tx_reducer::consume_aborted_txs(model::offset upto) {
+void tx_reducer::refresh_ongoing_aborted_txs(const model::record_batch& b) {


prepare batches are no longer used

What does this mean for old topics that have legacy prepare batches?

they were in pre-transactions-GA code ( > 2years ago), so we don't really support them anymore (after 22.3.0)

src/v/features/feature_table.h

.. into a separate utility so it can be shared by multiple code paths that parse this log data.

This helps clamp the max collectible offset on group partitions to not exceed earliest open trasnaction.

Prior to this change group_tx_fence shared the fence record batch type with data partitions (tx_fence). This made compaction logic complicated particularly because different compaction rules applied for fence batch in groups and data paritions. With the new feature, group fence has a separate dedicated batch type so it is easy to diambiguate both fence types.

Remove all control batch parsing. Control batches cannot be compacted (after redpanda-data#15404) and prepare batches are no longer used, most branches are dead code here. No logical changes.

We will add another transactional type for consumer offsets in a later commit. Renaming to disambiguate differnt transactional stms

Enables compaction of group transaction markers which was broker prior to this change. Group transaction batches are ommitted in the compaction reducer pass, check the code comments for more details.

The test generates interleavings of transactions across multiple groups in the same consumer offsets partition and repeatedly compacts and checks for some invariants.

ztlpn · 2024-06-28T18:07:08Z

src/v/storage/compaction_reducers.cc

+    // committed data has already been rewritten as separate raft_data batches,
+    // so no need to retain originally written group_prepare_tx batches while
+    // the transaction is in progress.
+    return is_compactible_control_batch(b.header().type);


So simple!

I wonder why we needed a separate batch type - if we know that we are in the consumer offsets topic, can't we just compact all fence batch types? Is this because in theory ordinary user transactions are also possible for the consumer offsets topic?

right, this is more of a code structuring problem because we don't have the "topic context" in all places, that is fixable with some (ugly) refactoring but even otherwise there is no reason to use the same batch type for fence in data & consumer offsets especially when every other batch type (data, commit/abort) are different. This change simplifies things because we can now just look at the batch header and say whether it is compactible and all the batch types are neatly grouped by topic (no overlap).

bharathv · 2024-06-28T19:47:21Z

Failures unrelated

dotnwat · 2024-06-28T21:34:38Z

src/v/kafka/server/group_data_parser.h

+    }
+
+private:
+    ss::future<> parse_fence(model::record_batch b) {


would probably be worth trying to move some of these implementations out of the header and into .cc files.

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

bharathv force-pushed the group_co_stm branch from e242c78 to 9cc1506 Compare June 20, 2024 20:24

bharathv force-pushed the group_co_stm branch from 9cc1506 to 0b3ecf2 Compare June 21, 2024 05:51

bharathv force-pushed the group_co_stm branch from 0b3ecf2 to 3e51813 Compare June 21, 2024 21:44

bharathv force-pushed the group_co_stm branch from 3e51813 to 3cb97b5 Compare June 22, 2024 17:27

bharathv force-pushed the group_co_stm branch 2 times, most recently from cb72c7f to 8e293de Compare June 23, 2024 23:04

bharathv marked this pull request as ready for review June 23, 2024 23:12

bharathv requested review from mmaslankaprv, andrwng, ztlpn and bashtanov June 24, 2024 02:22

mmaslankaprv reviewed Jun 25, 2024

View reviewed changes

src/v/kafka/server/group_tx_tracker_stm.h Outdated Show resolved Hide resolved

mmaslankaprv reviewed Jun 25, 2024

View reviewed changes

src/v/kafka/server/group_data_parser.h Show resolved Hide resolved

mmaslankaprv reviewed Jun 25, 2024

View reviewed changes

src/v/kafka/server/group_tx_tracker_stm.cc Show resolved Hide resolved

mmaslankaprv reviewed Jun 25, 2024

View reviewed changes

bharathv force-pushed the group_co_stm branch from 8e293de to 84102f7 Compare June 26, 2024 02:29

bharathv requested a review from mmaslankaprv June 26, 2024 02:30

bharathv force-pushed the group_co_stm branch from 84102f7 to c79294a Compare June 26, 2024 15:40

mmaslankaprv reviewed Jun 27, 2024

View reviewed changes

src/v/kafka/server/group_manager.h Outdated Show resolved Hide resolved

mmaslankaprv reviewed Jun 27, 2024

View reviewed changes

bharathv force-pushed the group_co_stm branch from c79294a to 49bb359 Compare June 27, 2024 06:28

bharathv requested a review from mmaslankaprv June 27, 2024 06:28

ztlpn reviewed Jun 27, 2024

View reviewed changes

andrwng reviewed Jun 27, 2024

View reviewed changes

bharathv force-pushed the group_co_stm branch from 49bb359 to 87b4716 Compare June 27, 2024 22:21

bharathv added this to the v24.2.1-rc1 milestone Jun 28, 2024

bharathv requested review from ztlpn and andrwng June 28, 2024 03:47

ztlpn reviewed Jun 28, 2024

View reviewed changes

src/v/features/feature_table.h Outdated Show resolved Hide resolved

bharathv added 7 commits June 28, 2024 07:44

group/tx/util: factor out group data parsing

0dc5c83

.. into a separate utility so it can be shared by multiple code paths that parse this log data.

group/tx: Add an stm that tracks open group transactions

660731c

This helps clamp the max collectible offset on group partitions to not exceed earliest open trasnaction.

compaction/tx_reducer: cleanup dead code

2b94ac0

Remove all control batch parsing. Control batches cannot be compacted (after redpanda-data#15404) and prepare batches are no longer used, most branches are dead code here. No logical changes.

stm/type: rename transactional to user_topic_transactional

d6802a0

We will add another transactional type for consumer offsets in a later commit. Renaming to disambiguate differnt transactional stms

group/tx/compaction: hook up compaction into group transactions

40e6472

Enables compaction of group transaction markers which was broker prior to this change. Group transaction batches are ommitted in the compaction reducer pass, check the code comments for more details.

group/tx/tests: test compaction with group transactions

167566e

The test generates interleavings of transactions across multiple groups in the same consumer offsets partition and repeatedly compacts and checks for some invariants.

bharathv force-pushed the group_co_stm branch from 87b4716 to 167566e Compare June 28, 2024 15:14

bharathv requested a review from ztlpn June 28, 2024 15:15

ztlpn approved these changes Jun 28, 2024

View reviewed changes

piyushredpanda merged commit c37412c into redpanda-data:dev Jun 28, 2024
15 of 18 checks passed

dotnwat reviewed Jun 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

group/transactions: compaction fixes #19931

group/transactions: compaction fixes #19931

bharathv commented Jun 20, 2024 •

edited

Loading

bharathv commented Jun 20, 2024

vbotbuildovich commented Jun 20, 2024 •

edited

Loading

bharathv commented Jun 20, 2024

vbotbuildovich commented Jun 20, 2024

bharathv commented Jun 21, 2024

bharathv commented Jun 21, 2024

bharathv commented Jun 22, 2024

mmaslankaprv Jun 25, 2024

bharathv Jun 26, 2024

mmaslankaprv Jun 25, 2024

bharathv Jun 25, 2024

bharathv commented Jun 26, 2024

mmaslankaprv Jun 27, 2024

ztlpn Jun 27, 2024

bharathv Jun 27, 2024

ztlpn Jun 27, 2024

bharathv Jun 27, 2024

ztlpn Jun 27, 2024

ztlpn Jun 27, 2024

bharathv Jun 27, 2024

ztlpn Jun 28, 2024

andrwng left a comment

andrwng Jun 27, 2024

bharathv Jun 27, 2024

andrwng Jun 27, 2024

bharathv Jun 27, 2024

andrwng Jun 27, 2024

bharathv Jun 27, 2024

ztlpn Jun 28, 2024

bharathv Jun 28, 2024

bharathv commented Jun 28, 2024

dotnwat Jun 28, 2024

group/transactions: compaction fixes #19931

group/transactions: compaction fixes #19931

Conversation

bharathv commented Jun 20, 2024 • edited Loading

Backports Required

Release Notes

Bug Fixes

bharathv commented Jun 20, 2024

vbotbuildovich commented Jun 20, 2024 • edited Loading

bharathv commented Jun 20, 2024

vbotbuildovich commented Jun 20, 2024

bharathv commented Jun 21, 2024

bharathv commented Jun 21, 2024

bharathv commented Jun 22, 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

bharathv commented Jun 26, 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

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

andrwng 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

bharathv commented Jun 28, 2024

Choose a reason for hiding this comment

bharathv commented Jun 20, 2024 •

edited

Loading

vbotbuildovich commented Jun 20, 2024 •

edited

Loading