Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rm_stm/tests: add test for producer expiration without data batches #18247

Merged
merged 2 commits into from
May 8, 2024

Conversation

bharathv
Copy link
Contributor

@bharathv bharathv commented May 3, 2024

In earlier versions we were not considering transactions without data
batches as candidates for expiry, thats not the case any more with
#18076, adding a regression test to future proof.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v24.1.x
  • v23.3.x
  • v23.2.x

Release Notes

  • none

@bharathv
Copy link
Contributor Author

bharathv commented May 3, 2024

23.3.x backport (with additional fix): #18248

fragmented_vector<model::producer_identity>
rm_stm::get_expired_producers() const {
fragmented_vector<model::producer_identity> result;
auto pids = std::views::keys(_log_state.ongoing_map);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we certain that using std::ranges is now blessed ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Judging from https://libcxx.llvm.org/Status/Cxx20.html ranges support should be good in clang 16.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I don't find this any more readable than a for loop :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see references to ranges elsewhere in the code.. I agree it is not any more readable than a range for but opens up possibilities to use std functions like find_if and such.

src/v/cluster/rm_stm.cc Outdated Show resolved Hide resolved
no logic changes, just refactoring
In earlier versions we were not considering transactions without data
batches as candidates for expiry, thats not the case any more with
redpanda-data#18076, adding a
regression test to future proof.
@bharathv
Copy link
Contributor Author

bharathv commented May 8, 2024

Failure test_raft_rpunit unrelated, looking at it separately.

@piyushredpanda piyushredpanda merged commit ee77be5 into redpanda-data:dev May 8, 2024
14 of 17 checks passed
@vbotbuildovich
Copy link
Collaborator

/backport v24.1.x

@vbotbuildovich
Copy link
Collaborator

/backport v23.3.x

@vbotbuildovich
Copy link
Collaborator

/backport v23.2.x

@vbotbuildovich
Copy link
Collaborator

Failed to create a backport PR to v23.3.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-18247-v23.3.x-487 remotes/upstream/v23.3.x
git cherry-pick -x dd22f58ff11650167f4de5eae577fe1e3a1de430 565c182c013ebbeb5abc997c8784006b5d480e4a

Workflow run logs.

@vbotbuildovich
Copy link
Collaborator

Failed to create a backport PR to v23.2.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-18247-v23.2.x-678 remotes/upstream/v23.2.x
git cherry-pick -x dd22f58ff11650167f4de5eae577fe1e3a1de430 565c182c013ebbeb5abc997c8784006b5d480e4a

Workflow run logs.

@vbotbuildovich
Copy link
Collaborator

Failed to create a backport PR to v24.1.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-18247-v24.1.x-357 remotes/upstream/v24.1.x
git cherry-pick -x dd22f58ff11650167f4de5eae577fe1e3a1de430 565c182c013ebbeb5abc997c8784006b5d480e4a

Workflow run logs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants