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

cloud_storage: Extend fast trim cleanup to new segment name #13158

Conversation

abhijat
Copy link
Contributor

@abhijat abhijat commented Aug 31, 2023

When a log segment is removed in fast trim, the associated index and tx files are also removed. This commit extends this feature to new log segment name formats, which may contain a term after the name.

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

Release Notes

  • none

@abhijat abhijat force-pushed the delete-support-files-for-all-segments-when-trim branch from c58a5da to 20417b1 Compare August 31, 2023 08:09
@abhijat abhijat marked this pull request as ready for review August 31, 2023 08:50
@abhijat abhijat requested review from Lazin, andijcr and VladLazar August 31, 2023 08:51
Comment on lines 532 to 533
const std::regex segment_expr{
R"#(.*\.log(\.\d+)?)#", std::regex_constants::optimize};
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we avoid constructing this regex on every invocation to trim_fast (e.g. make it a member)?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think we should also use re2, since it's already linked

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to anonymous namespace

Comment on lines 532 to 533
const std::regex segment_expr{
R"#(.*\.log(\.\d+)?)#", std::regex_constants::optimize};
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: a comment on segment formats would be nice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment, but I suspect there is a better way to document these expressions and strings which are spread around, perhaps the path name strings and associated patterns should live in a central specification namespace.

When a log segment is removed in fast trim, the associated index and tx
files are also removed. This commit extends this feature to new log
segment name formats, which may contain a term after the name.
@abhijat abhijat force-pushed the delete-support-files-for-all-segments-when-trim branch from 20417b1 to 56089f5 Compare August 31, 2023 10:33
@abhijat abhijat requested a review from VladLazar August 31, 2023 10:36
Copy link
Contributor

@VladLazar VladLazar left a comment

Choose a reason for hiding this comment

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

Shouldn't we backport to v23.2?

@abhijat abhijat merged commit 2e74d34 into redpanda-data:dev Aug 31, 2023
21 checks passed
@abhijat
Copy link
Contributor Author

abhijat commented Aug 31, 2023

Shouldn't we backport to v23.2?

I don't think this is a big enough change to backport, but if you think this is important or can cause a problem I can backport.

@VladLazar
Copy link
Contributor

Shouldn't we backport to v23.2?

I don't think this is a big enough change to backport, but if you think this is important or can cause a problem I can backport.

23.2 has a long life ahead of it, so I think we should backport fixes whenever possible.

@abhijat
Copy link
Contributor Author

abhijat commented Sep 1, 2023

/backport v23.2.x

@abhijat
Copy link
Contributor Author

abhijat commented Sep 1, 2023

23.2 has a long life ahead of it, so I think we should backport fixes whenever possible.

Started the backport

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.

3 participants