From 07e714fb21b2fa4ab412ef5082be88c292a244c9 Mon Sep 17 00:00:00 2001 From: alexey bashtanov Date: Wed, 8 May 2024 10:41:26 +0100 Subject: [PATCH] id_allocator: make it return a result<...> instead of a homemade type That's mostly for later use in raft_fixture::retry_with_leader --- src/v/cluster/id_allocator_frontend.cc | 16 +++++------ src/v/cluster/id_allocator_stm.cc | 30 +++++++++++--------- src/v/cluster/id_allocator_stm.h | 5 +--- src/v/cluster/tests/id_allocator_stm_test.cc | 6 ++-- 4 files changed, 28 insertions(+), 29 deletions(-) diff --git a/src/v/cluster/id_allocator_frontend.cc b/src/v/cluster/id_allocator_frontend.cc index dbf408551818..541881662967 100644 --- a/src/v/cluster/id_allocator_frontend.cc +++ b/src/v/cluster/id_allocator_frontend.cc @@ -121,11 +121,11 @@ reset_id_handler::process(ss::shard_id shard, reset_id_allocator_request req) { } return stm->reset_next_id(id, timeout) .then([](id_allocator_stm::stm_allocation_result r) { - if (r.raft_status != raft::errc::success) { + if (!r) { vlog( clusterlog.warn, "allocate id stm call failed with {}", - raft::make_error_code(r.raft_status).message()); + r.assume_error().message()); return reset_id_allocator_reply{errc::replication_error}; } @@ -146,7 +146,7 @@ allocate_id_handler::process(ss::shard_id shard, allocate_id_request req) { "can't get partition by {} ntp", model::id_allocator_ntp); return ss::make_ready_future( - allocate_id_reply{0, errc::topic_not_exists}); + 0, errc::topic_not_exists); } auto stm = partition->id_allocator_stm(); if (!stm) { @@ -155,19 +155,19 @@ allocate_id_handler::process(ss::shard_id shard, allocate_id_request req) { "can't get id allocator stm of the {}' partition", model::id_allocator_ntp); return ss::make_ready_future( - allocate_id_reply{0, errc::topic_not_exists}); + 0, errc::topic_not_exists); } return stm->allocate_id(timeout).then( [](id_allocator_stm::stm_allocation_result r) { - if (r.raft_status != raft::errc::success) { + if (!r) { vlog( clusterlog.warn, "allocate id stm call failed with {}", - raft::make_error_code(r.raft_status).message()); - return allocate_id_reply{r.id, errc::replication_error}; + r.assume_error().message()); + return allocate_id_reply{-1, errc::replication_error}; } - return allocate_id_reply{r.id, errc::success}; + return allocate_id_reply{r.assume_value(), errc::success}; }); }); } diff --git a/src/v/cluster/id_allocator_stm.cc b/src/v/cluster/id_allocator_stm.cc index 19a86a9a7561..217fe333fdea 100644 --- a/src/v/cluster/id_allocator_stm.cc +++ b/src/v/cluster/id_allocator_stm.cc @@ -76,27 +76,28 @@ id_allocator_stm::reset_next_id( return _lock .with( timeout, [this, id, timeout]() { return advance_state(id, timeout); }) - .handle_exception_type([](const ss::semaphore_timed_out&) { - return stm_allocation_result{-1, raft::errc::timeout}; - }); + .handle_exception_type( + [](const ss::semaphore_timed_out&) -> stm_allocation_result { + return raft::make_error_code(raft::errc::timeout); + }); } ss::future id_allocator_stm::advance_state( int64_t value, model::timeout_clock::duration timeout) { if (!co_await sync(timeout)) { - co_return stm_allocation_result{-1, raft::errc::timeout}; + co_return raft::make_error_code(raft::errc::timeout); } if (value < _curr_id) { - co_return stm_allocation_result{_curr_id, raft::errc::success}; + co_return _curr_id; } _curr_id = value; auto success = co_await set_state(_curr_id + _batch_size, timeout); if (!success) { - co_return stm_allocation_result{-1, raft::errc::timeout}; + co_return raft::make_error_code(raft::errc::timeout); } _curr_batch = _batch_size; - co_return stm_allocation_result{_curr_id, raft::errc::success}; + co_return stm_allocation_result(_curr_id); } ss::future id_allocator_stm::set_state( @@ -123,31 +124,32 @@ ss::future id_allocator_stm::allocate_id(model::timeout_clock::duration timeout) { return _lock .with(timeout, [this, timeout]() { return do_allocate_id(timeout); }) - .handle_exception_type([](const ss::semaphore_timed_out&) { - return stm_allocation_result{-1, raft::errc::timeout}; - }); + .handle_exception_type( + [](const ss::semaphore_timed_out&) -> stm_allocation_result { + return raft::make_error_code(raft::errc::timeout); + }); } ss::future id_allocator_stm::do_allocate_id(model::timeout_clock::duration timeout) { if (!co_await sync(timeout)) { - co_return stm_allocation_result{-1, raft::errc::timeout}; + co_return raft::make_error_code(raft::errc::timeout); } if (_curr_batch == 0) { _curr_id = _state; if (!co_await set_state(_curr_id + _batch_size, timeout)) { - co_return stm_allocation_result{-1, raft::errc::timeout}; + co_return raft::make_error_code(raft::errc::timeout); } _curr_batch = _batch_size; } - auto id = _curr_id; + int64_t id = _curr_id; _curr_id += 1; _curr_batch -= 1; - co_return stm_allocation_result{id, raft::errc::success}; + co_return stm_allocation_result{id}; } ss::future<> id_allocator_stm::apply(const model::record_batch& b) { diff --git a/src/v/cluster/id_allocator_stm.h b/src/v/cluster/id_allocator_stm.h index a0d693eb2abf..97d0f5d3e757 100644 --- a/src/v/cluster/id_allocator_stm.h +++ b/src/v/cluster/id_allocator_stm.h @@ -34,10 +34,7 @@ class id_allocator_stm final : public raft::persisted_stm<> { public: static constexpr std::string_view name = "id_allocator_stm"; - struct stm_allocation_result { - int64_t id; - raft::errc raft_status{raft::errc::success}; - }; + using stm_allocation_result = result; explicit id_allocator_stm(ss::logger&, raft::consensus*); diff --git a/src/v/cluster/tests/id_allocator_stm_test.cc b/src/v/cluster/tests/id_allocator_stm_test.cc index 782420216dfb..b30c38c3b258 100644 --- a/src/v/cluster/tests/id_allocator_stm_test.cc +++ b/src/v/cluster/tests/id_allocator_stm_test.cc @@ -60,10 +60,10 @@ struct id_allocator_stm_fixture : simple_raft_fixture { for (int i = 0; i < n; i++) { auto result = _stm->allocate_id(1s).get0(); - BOOST_REQUIRE_EQUAL(raft::errc::success, result.raft_status); - BOOST_REQUIRE_LT(cur_last_id, result.id); + BOOST_REQUIRE_EQUAL(result.has_value(), true); + BOOST_REQUIRE_LT(cur_last_id, result.assume_value()); - cur_last_id = result.id; + cur_last_id = result.assume_value(); } return cur_last_id; }