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

tests: improve CloudRetentionTest #10327

Merged
merged 3 commits into from
Apr 26, 2023

Conversation

jcsp
Copy link
Contributor

@jcsp jcsp commented Apr 25, 2023

This test was:
- Checking for retention enforcement in a way that
  overlapped with ongoing uploads, so had unpredictable
  runtime.
- Checking total object size rather than logical partition
  size, so its stability was influenced by extra objects
  like indices.

While working on this, I also noticed some logging issues with the archival housekeeping that I've fixed opportunistically.

Fixes #10282

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

Release Notes

  • none

This was omitting the actual exception.
Avoid emitting WARN log during shutdown.
This test was:
- Checking for retention enforcement in a way that
  overlapped with ongoing uploads, so had unpredictable
  runtime.
- Checking total object size rather than logical partition
  size, so its stability was influenced by extra objects
  like indices.

Fixes redpanda-data#10282
@jcsp jcsp added kind/bug Something isn't working area/tests labels Apr 25, 2023
@jcsp
Copy link
Contributor Author

jcsp commented Apr 26, 2023

@jcsp jcsp marked this pull request as ready for review April 26, 2023 09:34
@jcsp jcsp requested review from VladLazar and abhijat April 26, 2023 09:34
@@ -1806,7 +1806,7 @@ ss::future<> ntp_archiver::housekeeping() {
co_await garbage_collect();
}
} catch (std::exception& e) {
vlog(_rtclog.warn, "Error occured during housekeeping", e.what());
vlog(_rtclog.warn, "Error occurred during housekeeping: {}", e.what());
Copy link
Contributor

Choose a reason for hiding this comment

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

We should really write a clang-tidy check for this ...

Copy link
Member

@dotnwat dotnwat May 2, 2023

Choose a reason for hiding this comment

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

did someone say clang-tidy check?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. It would be great if we had a check that parsed vlog and format invocations and ensured the number of placeholders ({}) matches the number of args provided.

Copy link
Member

Choose a reason for hiding this comment

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

yeh that would be cool. IIUC this has been asked for from libfmt but apparently it isn't possible. clang-tidy does seem like a reasonable solution

@jcsp jcsp merged commit 9ed8bd7 into redpanda-data:dev Apr 26, 2023
@jcsp jcsp deleted the issue-10282-cloud-retention-test branch April 28, 2023 16:37
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI Failure (Timeout) in cloud_retention_test.CloudRetentionTest.test_cloud_retention
3 participants