rm stm state cleanup #18277

bharathv · 2024-05-07T01:22:34Z

Main changes in this PR

Makes the state machine rm_stm lean by moving all the types to a separate header/cc
Moves all transaction related state to cluster::tx namespace from cluster
Removes snapshot v3 related dead code.

The change is purely mechanical. This preps the code for the next PR that moves even more state out of the state machine. The end goal is to make the state machine lean and easy to reason about.

Backports Required

Release Notes

none

bharathv · 2024-05-07T01:24:20Z

/dt

vbotbuildovich · 2024-05-07T03:54:03Z

new failures in https://buildkite.com/redpanda/redpanda/builds/48762#018f50fd-d113-4dd7-8ff3-f4ce242ba3b7:

"rptest.tests.e2e_shadow_indexing_test.EndToEndThrottlingTest.test_throttling.cloud_storage_type=CloudStorageType.S3"

new failures in https://buildkite.com/redpanda/redpanda/builds/48762#018f50fd-d115-4dd2-8caf-73486656d256:

"rptest.tests.controller_snapshot_test.ControllerSnapshotPolicyTest.test_upgrade_auto_enable"

new failures in https://buildkite.com/redpanda/redpanda/builds/48762#018f50fd-d10f-4469-a349-bb63573fb6ef:

"rptest.tests.e2e_shadow_indexing_test.EndToEndThrottlingTest.test_throttling.cloud_storage_type=CloudStorageType.ABS"

new failures in https://buildkite.com/redpanda/redpanda/builds/48762#018f5105-93e0-4313-ac3e-84f231967828:

"rptest.tests.e2e_shadow_indexing_test.EndToEndThrottlingTest.test_throttling.cloud_storage_type=CloudStorageType.S3"

new failures in https://buildkite.com/redpanda/redpanda/builds/48762#018f5105-93db-4385-b29f-250fa99d2215:

"rptest.tests.controller_snapshot_test.ControllerSnapshotPolicyTest.test_upgrade_auto_enable"

new failures in https://buildkite.com/redpanda/redpanda/builds/48762#018f5105-93de-4c27-bc31-6b015249222d:

"rptest.tests.e2e_shadow_indexing_test.EndToEndThrottlingTest.test_throttling.cloud_storage_type=CloudStorageType.ABS"

new failures in https://buildkite.com/redpanda/redpanda/builds/49048#018f7458-6b5a-4989-9551-f49a77ada713:

"rptest.tests.cloud_storage_scrubber_test.CloudStorageScrubberTest.test_scrubber.cloud_storage_type=CloudStorageType.ABS"

vbotbuildovich · 2024-05-07T03:59:18Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/48762#018f50fd-d115-4dd2-8caf-73486656d256

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/48902#018f5ee9-4f9c-4c64-82c1-fb531c0f71ac

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/48908#018f5fbf-04fa-46ef-b2bc-4c8ce13a0daa

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/49048#018f7458-6b61-46a3-a052-36a386140624

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/49048#018f7458-6b5f-49ce-a558-faa18ac402a0

bharathv · 2024-05-09T18:46:50Z

/dt

bashtanov · 2024-05-13T09:48:07Z

src/v/features/feature_table.h

@@ -99,7 +99,8 @@ inline const std::unordered_set<std::string_view> retired_features = {
  "mtls_authentication",
  "rm_stm_kafka_cache",
  "transaction_ga",
-};
+  "idempotency_v2",
+  "transaction_partitioning"};


how do you make sure the feature flag is not in use anymore?

https://redpandadata.atlassian.net/wiki/spaces/CORE/pages/170491930/Features+Cluster+Logical+Versioning is relevant. Once the cluster is fully upgraded, the feature is always true and this coupled with no rolling back requirement across minors guarantees it is always true.

so how do you know all clusters are upgraded so that the feature is always true on all of them?

bashtanov · 2024-05-13T09:49:35Z

src/v/cluster/rm_stm.cc

-  model::tx_seq tx_seq,
-  std::chrono::milliseconds transaction_timeout_ms,
-  model::partition_id tm) {
-    if (is_transaction_partitioning()) {


it looks like the last use of function is_transaction_partitioning, should we remove it?

bashtanov · 2024-05-13T09:50:46Z

src/v/cluster/tests/rm_stm_compatibility_test.cc

+
+    return std::move(builder).build();
+}
+
 SEASTAR_THREAD_TEST_CASE(fence_batch_compatibility) {
    vlog(logger.info, "Test fence_batch_v1");


why do we still need it?

we can still read old fence batch versions in the log after the upgrade. (eg: if old data is not truncated).

src/v/features/feature_table.h

bharathv

Added an extra commit on top (that was supposed to come in the next PR) since it is mechanical logging change, so thought it'd make sense to lump with this one.

src/v/features/feature_table.h

bharathv · 2024-05-13T18:31:12Z

src/v/cluster/tests/rm_stm_compatibility_test.cc

+
+    return std::move(builder).build();
+}
+
 SEASTAR_THREAD_TEST_CASE(fence_batch_compatibility) {
    vlog(logger.info, "Test fence_batch_v1");


we can still read old fence batch versions in the log after the upgrade. (eg: if old data is not truncated).

bharathv · 2024-05-13T18:32:11Z

src/v/features/feature_table.h

@@ -99,7 +99,8 @@ inline const std::unordered_set<std::string_view> retired_features = {
  "mtls_authentication",
  "rm_stm_kafka_cache",
  "transaction_ga",
-};
+  "idempotency_v2",
+  "transaction_partitioning"};


https://redpandadata.atlassian.net/wiki/spaces/CORE/pages/170491930/Features+Cluster+Logical+Versioning is relevant. Once the cluster is fully upgraded, the feature is always true and this coupled with no rolling back requirement across minors guarantees it is always true.

These are always true by default going forward.

src/v/cluster/tests/rm_stm_tests.cc

src/v/cluster/producer_state.h

src/v/cluster/rm_stm_types.h

snapshot version=5 is the active version since 23.3.x, Retiring code related to snapshot version=3.

Next change consolidates more types in this header

consolidate under cluster::tx so we donot polluter top level ns.

Break the monolith of rm_stm further and move unnecessarly logic out of it.

bharathv · 2024-05-14T04:52:11Z

Failures unrelated:

ztlpn · 2024-05-14T14:34:59Z

src/v/cluster/rm_stm_types.h

+    }
+};
+
+struct transaction_info {


nit: now that this is in tx namespace, the name transaction_info has additional gravity :) Can we modify the name to hint that this is about a single partition?

ya, this is changing in the next PR, trying to cleanup a lot of stuff in this area.

github-actions bot added the area/redpanda label May 7, 2024

bharathv force-pushed the rm_stm_type_cleanup branch from efe0525 to dd9a388 Compare May 9, 2024 18:43

bharathv changed the title ~~Rm stm type cleanup~~ rm stm state cleanup May 9, 2024

bharathv marked this pull request as ready for review May 9, 2024 22:39

bharathv requested review from mmaslankaprv, bashtanov and ztlpn May 9, 2024 22:39

bashtanov reviewed May 13, 2024

View reviewed changes

mmaslankaprv reviewed May 13, 2024

View reviewed changes

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

bharathv commented May 13, 2024

View reviewed changes

rm_stm: retire idempotency_v2 and transaction_partitioning

0596ff8

These are always true by default going forward.

bharathv force-pushed the rm_stm_type_cleanup branch from dd9a388 to 8f04fd1 Compare May 13, 2024 18:37

ztlpn reviewed May 13, 2024

View reviewed changes

src/v/cluster/tests/rm_stm_tests.cc Outdated Show resolved Hide resolved

ztlpn reviewed May 13, 2024

View reviewed changes

src/v/cluster/producer_state.h Outdated Show resolved Hide resolved

ztlpn reviewed May 13, 2024

View reviewed changes

src/v/cluster/rm_stm_types.h Outdated Show resolved Hide resolved

bharathv added 5 commits May 13, 2024 14:00

tx/snapshot: remove snapshot dead code

96bf512

snapshot version=5 is the active version since 23.3.x, Retiring code related to snapshot version=3.

tx/types: rename tx_snapshot_utils to rm_stm_types

8c02dfa

Next change consolidates more types in this header

types: move all rm_stm types to cluster::tx ns

f742f58

consolidate under cluster::tx so we donot polluter top level ns.

tx/types: move batch parsing logic into types header

b76edcd

Break the monolith of rm_stm further and move unnecessarly logic out of it.

tx/producer_state: move to cluster::tx

63bc2c0

bharathv force-pushed the rm_stm_type_cleanup branch from 8f04fd1 to 63bc2c0 Compare May 13, 2024 22:33

bharathv requested review from bashtanov, mmaslankaprv and ztlpn May 14, 2024 04:50

bashtanov approved these changes May 14, 2024

View reviewed changes

ztlpn reviewed May 14, 2024

View reviewed changes

ztlpn approved these changes May 14, 2024

View reviewed changes

piyushredpanda merged commit d76e9a6 into redpanda-data:dev May 14, 2024
14 of 17 checks passed

bharathv deleted the rm_stm_type_cleanup branch May 14, 2024 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rm stm state cleanup #18277

rm stm state cleanup #18277

bharathv commented May 7, 2024 •

edited

bharathv commented May 7, 2024

vbotbuildovich commented May 7, 2024 •

edited

vbotbuildovich commented May 7, 2024 •

edited

bharathv commented May 9, 2024

bashtanov May 13, 2024

bharathv May 13, 2024

bashtanov May 14, 2024

bashtanov May 13, 2024

bashtanov May 13, 2024

bharathv May 13, 2024

bharathv left a comment

bharathv May 13, 2024

bharathv May 13, 2024

bharathv commented May 14, 2024

ztlpn May 14, 2024

bharathv May 14, 2024

rm stm state cleanup #18277

rm stm state cleanup #18277

Conversation

bharathv commented May 7, 2024 • edited

Backports Required

Release Notes

bharathv commented May 7, 2024

vbotbuildovich commented May 7, 2024 • edited

vbotbuildovich commented May 7, 2024 • edited

bharathv commented May 9, 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

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

bharathv commented May 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bharathv commented May 7, 2024 •

edited

vbotbuildovich commented May 7, 2024 •

edited

vbotbuildovich commented May 7, 2024 •

edited