[transaction] Clear old pids from rm_stm state in overflow #7057

VadimPlh · 2022-11-02T14:33:15Z

Cover letter

Problem

User can use transaction/idempotency in different way. Some of them can be cause of problem. For example: user can create producer per request. Idempotency in kafka protocol works per session, the session is scoped by a producer so if user create a producer per request we will store all info for each producer in Idempotency case. It does not provide any benefits for user, but will create big maps in internal state for rm_stm. Also the same things for transaction.

We need to prevent this type overflow. So idea is add new settings (max_concurrent_producer_ids). And when rm_stm will reach this limits we will spawn clear process for log and mame state.

Fix

For Idempotency we use _log_state.seq_table, where we store session info. The simplest way to track order of last apply_data for pid. We will store seq_entry inside intrusive list to store replication order.

For transaction is a little bit harder. We can not delete any info for ongoing transaction. But we can understand do user finish transaction or not, For this we just need to check _log_state.tx_seqs. Because on abort or commit we clear this map. So for all commited/aborted txs we just can delete all info from internal maps

Fixes #5321

Backport Required

UX changes

none

Release notes

Features

Adding new redpanda setting (max_concurrent_producer_ids) to control how much sessions for transaction/idempotency will be saved

rystsov

tx houskeeping looks good while the idempotency has issues. also please add tests for the idempotency, something like:

create a producer, produce
create n producers to exhaust the limit and kick out the first producer
produce via the first producer
observe error

src/v/cluster/rm_stm.cc

src/v/cluster/rm_stm.h

src/v/cluster/rm_stm.cc

src/v/cluster/rm_stm.h

src/v/cluster/rm_stm.cc

mmaslankaprv · 2022-11-04T13:13:43Z

I am wondering if we want to remove the oldest PIDs from all of them ?)
If so we may not need to keep another index but instead use an intrusive list to keep track of an insertion order and add a timestamp to existing object this way we would iterate (according to insertion order) up to the point where PIDs are more recent than the the requested timestamp and remove all previous (or if it is just number of elements then we wouldn't even have to track timestamp).

Similar approach is used in fetch_session.h

src/v/cluster/rm_stm.cc

rystsov

I think we need @dotnwat's feedback on the name of the property but otherwise it looks good!

rystsov · 2022-12-01T19:59:12Z

src/v/cluster/rm_stm.cc

@@ -2232,6 +2234,8 @@ void rm_stm::apply_data(model::batch_identity bid, model::offset last_offset) {
                  _log_state.lru_idempotent_pids.iterator_to(seq_it->second));
            }
            _log_state.lru_idempotent_pids.push_back(seq_it->second);
+            spawn_background_clean_for_pids(


nit: drop rm_stm::? sometimes you use clear_type sometimes not

dotnwat · 2022-12-01T21:27:24Z

I think we need @dotnwat's feedback on the name of the property but otherwise it looks good!

@rystsov I chimed in on that comment thread. but i wasn't thinking it was about the name, but rather the default value.

We need to prevent overflow for some state maps, so we need to delete info for pids which does not have tx. For this we just should check is pid inside tx_seqs map or not. If not it means tx was commited or aborted

To track old session for idempotent request we need to store replication order for them for delete old session in right order.

So idea is clear internal state for oldest session if we have overflow. we just need to delete info for oldest pid in out state

New option will control the max size of internal maps in rm_stm. User can use transaction/idempotency in different way. Some of them can be cause of problem. For example: user can create producer per request. Idempotency in kafka protocol works per session, the session is scoped by a producer so if user create a producer per request we will store all info for each producer in Idempotency case. It does not provide any benefits for user, but will create big maps in internal state for rm_stm. Also the same things for transaction.

Run cleaning for old session in replicate_seq and apply_data

dotnwat · 2022-12-02T19:38:51Z

Failures

VadimPlh · 2022-12-02T23:12:58Z

/backport v22.3.x

github-actions bot added the area/redpanda label Nov 2, 2022

VadimPlh changed the title ~~Issue 5321~~ [transaction] Clear old pids from rm_stm state in overflow Nov 3, 2022

mmedenjak added kind/bug Something isn't working area/raft and removed area/redpanda labels Nov 3, 2022

VadimPlh force-pushed the issue-5321 branch from 9f399b0 to c62da63 Compare November 3, 2022 15:20

github-actions bot added the area/redpanda label Nov 3, 2022

VadimPlh force-pushed the issue-5321 branch 3 times, most recently from 2e116dc to 43dde1b Compare November 3, 2022 16:47

VadimPlh marked this pull request as ready for review November 3, 2022 16:48

VadimPlh requested review from rystsov and bharathv November 3, 2022 16:48

mmedenjak requested a review from mmaslankaprv November 3, 2022 18:48

piyushredpanda added this to the v22.3.1-rc5 milestone Nov 4, 2022

rystsov reviewed Nov 4, 2022

View reviewed changes

src/v/cluster/rm_stm.cc Show resolved Hide resolved

src/v/cluster/rm_stm.cc Outdated Show resolved Hide resolved

src/v/cluster/rm_stm.cc Outdated Show resolved Hide resolved

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