cloud_storage: add partition manifest downloader #20314
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WillemKauf
previously approved these changes
Jun 27, 2024
andrwng
dismissed
WillemKauf’s stale review
The merge-base changed after approval.
There are various calls to download partition manifests (e.g. read replicas, partition recovery, Raft snapshot recovery) where the application doesn't necessarily care about the format or path of the manifest. These callers typically use remote::try_download_partition_manifest(), which checks for both the binary and JSON manifest. Since the naming scheme of objects is changing, this introduces a downloader that performs this same operation, but also takes into account the naming scheme. A later commit will plug this into the various users to replace try_download_partition_manifest(). I considered moving this logic into the remote, but generally the remote seems to have grown crowded with business logic. Naming scheme seems like it should fall outside of the remote's concern, considering most other methods in remote rely on callers to pass a path.
Topic recovery validation checks if partition manifests exist, and use a method that's built into the cloud_storage::remote. In the effort to change our object naming scheme, this means this needs to be updated to support the new naming. Rather than adding more business logic into the remote, this adds a method to the new partition_manifest_downloader to check if a given partition's manifest exists. A later commit will plumb this into topic recovery validation.
andrwng
force-pushed
the
labels-partition-downloader
branch
from
aa96693
to
c5f5f80
Compare
|
Latest push is just a rebase |
dotnwat
approved these changes
Jun 28, 2024
Comment on lines
+58
to
+61
| const cloud_storage_clients::bucket_name bucket_; | ||
| const remote_path_provider& remote_path_provider_; | ||
| const model::ntp ntp_; | ||
| const model::initial_revision_id rev_; |
There was a problem hiding this comment.
nit: does clang-tidy not complain about these const members in a movable class? https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.html
There was a problem hiding this comment.
Ah it does. Movability aside I generally prefer to make things const when they can be though, for readability.
Interestingly though it still complains if I make it non-movable non-copyable. I'll leave this as is for now, unless you feel strongly about it
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are various calls to download partition manifests (e.g. read replicas,
partition recovery, Raft snapshot recovery) where the application
doesn't necessarily care about the format or path of the manifest. These
callers typically use remote::try_download_partition_manifest(), which
checks for both the binary and JSON manifest.
Since the naming scheme of objects is changing, this introduces a
downloader that performs this same operation, but also takes into
account the naming scheme.
A later commit will plug this into the various users to replace
try_download_partition_manifest().
I considered moving this logic into the remote, but generally the remote
seems to have grown crowded with business logic. Naming scheme seems
like it should fall outside of the remote's concern, considering most
other methods in remote rely on callers to pass a path.
A similar method is added for checking if manifests exist, to eventually
subsume
remote::partition_manifest_exists()for the same reason.Backports Required
Release Notes