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

archival: skip spillover retention if not collectable #23358

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

andrwng
Copy link
Contributor

@andrwng andrwng commented Sep 18, 2024

Compact-only topics should never apply retention. This wasn't the case for the spillover region in tiered storage.

Note, for the STM region, we do correctly no-op when trying to apply retention for compacted topics.

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.2.x
  • v24.1.x
  • v23.3.x

Release Notes

Bug Fixes

  • Fixes a bug that would cause retention to be applied to compact-only tiered storage topics.

Compact-only topics should never apply retention. This wasn't the case
for the spillover region in tiered storage.

Note, for the STM region, we do correctly no-op when trying to apply
retention for compacted topics.
@vbotbuildovich
Copy link
Collaborator

ASSERT_GE(total_records, 500);
ASSERT_TRUE(archiver.sync_for_tests().get());
archiver.apply_spillover().get();
ss::sleep(5s).get();
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice to document why this sleep is needed or better wait for the specific condition we are looking for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will remove in a follow-up. Want to merge this one ASAP

.set_value(std::make_optional<size_t>(5));
test_local_cfg.get("cloud_storage_spillover_manifest_size")
.set_value(std::optional<size_t>{});
test_local_cfg.get("log_retention_ms")
Copy link
Contributor

Choose a reason for hiding this comment

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

can we do log_retention_bytes to avoid relying on timers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do in a follow-up. Want to merge this one ASAP

@lf-rep lf-rep merged commit 76ea911 into redpanda-data:dev Sep 18, 2024
18 of 19 checks passed
@vbotbuildovich
Copy link
Collaborator

/backport v24.2.x

@vbotbuildovich
Copy link
Collaborator

/backport v24.1.x

@vbotbuildovich
Copy link
Collaborator

/backport v23.3.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-23358-v23.3.x-523 remotes/upstream/v23.3.x
git cherry-pick -x 26df815465dc2cfd68e716e0936d1bdc99bb248b

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-23358-v24.1.x-222 remotes/upstream/v24.1.x
git cherry-pick -x 26df815465dc2cfd68e716e0936d1bdc99bb248b

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.

5 participants