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

cst/inv: Minor fixes for issues found during scrubber integration #23240

Merged
merged 2 commits into from
Sep 9, 2024

Conversation

abhijat
Copy link
Contributor

@abhijat abhijat commented Sep 9, 2024

This PR contains two minor fixes for issues found during scrubber integration:

  1. fix an unchecked optional access in inv. consumer
  2. allow uuid prefixes to segment paths

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

  • none

@@ -35,7 +35,7 @@ namespace views = std::views;

namespace {
// hash-string/ns/tp/partition_rev/.*
Copy link
Contributor

@nvartolomei nvartolomei Sep 9, 2024

Choose a reason for hiding this comment

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

comment is outdated now, it doesn't mention uuid as a possible option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated comment

@@ -43,6 +43,8 @@ TEST(Consumer, ParseNTPFromPath) {
std::vector<p> test_data{
{"a0a6eeb8/kafka/topic-x/999_24/178-188-1574137-1-v1.log.1",
std::make_optional(make_ntp("kafka", "topic-x", 999))},
{"d10492a6-2408-418e-9b6b-051697c5255b/k/t/1_24/---",
Copy link
Contributor

Choose a reason for hiding this comment

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

is this how the real path looks? i suppose this is the cluster uuid? don't we still include the hash prefix too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I discovered similar paths while running ducktape test:

d10492a6-2408-418e-9b6b-051697c5255b/kafka/topic-kppbtczbbq/2_22/429-500-1153657-1-v1.log.1

and looking at the code leads to https://github.com/redpanda-data/redpanda/blob/dev/src/v/cloud_storage/remote_path_provider.cc#L143 and https://github.com/redpanda-data/redpanda/blob/dev/src/v/cloud_storage/segment_path_utils.cc#L20

If there is a label it looks like we use the cluster uuid, otherwise a hash prefix

Since the segment prefix has been changed recently to either contain
cluster uuid or the regular hash, the path matching expression is
adjusted to also accept UUID.
@abhijat abhijat force-pushed the feat/inv-scrub/misc-fixes branch from bb95b26 to d709a8f Compare September 9, 2024 11:08
@abhijat abhijat requested a review from nvartolomei September 9, 2024 11:09
@vbotbuildovich
Copy link
Collaborator

@abhijat abhijat merged commit 64c8390 into redpanda-data:dev Sep 9, 2024
17 checks passed
@vbotbuildovich
Copy link
Collaborator

/backport v24.2.x

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