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: minor fixes in topic_mount_handler #22676

Merged
merged 1 commit into from
Aug 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions src/v/cloud_storage/tests/topic_mount_handler_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,15 @@ static const model::topic_namespace test_tp_ns_override{
static ss::abort_source never_abort;
static constexpr model::cloud_credentials_source config_file{
model::cloud_credentials_source::config_file};

static cluster::topic_configuration
get_topic_configuration(cluster::topic_properties topic_props) {
auto topic_cfg = cluster::topic_configuration(
test_tp_ns.ns, test_tp_ns.tp, 1, 1);
topic_cfg.properties = std::move(topic_props);
return topic_cfg;
}

} // namespace

struct TopicMountHandlerFixture
Expand All @@ -64,14 +73,6 @@ struct TopicMountHandlerFixture
pool.stop().get();
}

cluster::topic_configuration
get_topic_configuration(cluster::topic_properties topic_props) const {
auto topic_cfg = cluster::topic_configuration(
test_tp_ns.ns, test_tp_ns.tp, 1, 1);
topic_cfg.properties = std::move(topic_props);
return topic_cfg;
}

ss::sharded<cloud_storage_clients::client_pool> pool;
ss::sharded<remote> remote;
};
Expand Down
6 changes: 6 additions & 0 deletions src/v/cloud_storage/topic_mount_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ ss::future<topic_mount_result> topic_mount_handler::check_mount(
_bucket, manifest_path, parent, existence_check_type::manifest);

// If the manifest exists, it is possible to mount the topic.
// If the result is anything but download_result::success, it is
// assumed that the mount manifest does not exist, or is unreachable for
// whatever reason. In that case, it is simple enough for the user to
// reissue a request to `mount_topic()`.
co_return (exists_result == download_result::success)
? topic_mount_result::success
: topic_mount_result::mount_manifest_does_not_exist;
Expand All @@ -72,6 +76,8 @@ ss::future<topic_mount_result> topic_mount_handler::commit_mount(
const auto delete_result = co_await _remote.delete_object(
_bucket, manifest_path, parent);

// If the upload_result is anything but upload_result::success,
// the mount manifest was not deleted, and we cannot mount the topic.
co_return (delete_result == upload_result::success)
? topic_mount_result::success
: topic_mount_result::mount_manifest_not_deleted;
Expand Down
2 changes: 1 addition & 1 deletion src/v/cloud_storage/topic_mount_handler.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ class topic_mount_handler {
ss::future<topic_mount_result> mount_topic(
const cluster::topic_configuration& topic_cfg, retry_chain_node& parent);

// Perform the mounting process by deleting the topic mount manifest.
// Perform the unmounting process by uploading the topic mount manifest.
// topic_cfg should be the recovered topic configuration from a topic
// manifest in cloud storage. If it has a value, the remote_label stored in
// the topic properties is used as the "source" label. Otherwise, the
Expand Down
Loading