mirror of
https://github.com/scylladb/scylladb.git
synced 2026-04-26 03:20:37 +00:00
db77b5bd03
This is a followup to #19937, for #19803. See in particular [this comment](https://github.com/scylladb/scylladb/issues/19803#issuecomment-2258371923). The primary conversion target is coroutines. However, while coroutines are the most convenient style, they are only infrequently usable in this case, for the following reasons: - Wherever we have a `future::finally()` that calls a cleanup function that returns a future (which must be awaited), we cannot use `co_await`. We can only use `seastar::async()` with `deferred_close` or `defer()`. - The code passes lots of lambdas, and `co_await` cannot be used in lambdas. First, I tried, and the compiler rejects it; second, a capturing lambda that is a coroutine is a trap [[1]](https://devblogs.microsoft.com/oldnewthing/20211103-00/?p=105870) [[2]](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rcoro-capture). In most cases, I didn't have to use naked `seastar::async()`; there were specialized wrappers in place already. Thus, most of the changes target `seastar::thread` context under existent `seastar::async()` wrappers, and only a few functions end up as coroutines. The last patch in the series (`test/sstable: remove useless variable from promoted_index_read()`) is an independent micro-cleanup, the opportunity for which I thought to have noticed while reading the code. The tail of `test/boost/sstable_test.cc` (the stuff following `promoted_index_read()`) is already written as `seastar::thread`. That's already better (for readability) than future chaining; but could have I perhaps further converted those functions to coroutines? My answer was "no": - Some of the candidate functions relied on deferred cleanups that might need to yield (all three variants of `count_rows()`). - Some had been implemented by passing lambdas to wrappers of `seastar::async()` (`sub_partition_read()`, `sub_partitions_read()`). - The test case `test_skipping_in_compressed_stream()` initially looked promising for co-routinization (from its starting point `seastar::async()`), because it seemed to employ no deferred cleanup (that might need to yield). However, the function uses three lambdas that must be able to yield internally, and one of those (`make_is()`) is even capturing. - The rest (`test_empty_key_view_comparison()`, `test_parse_path_good()`, `test_parse_path_bad()`) was synchronous code to begin with. ``` test/boost/sstable_test.cc | 188 +++++++++----------- 1 file changed, 83 insertions(+), 105 deletions(-) ``` Refactoring; no backport needed. Closes scylladb/scylladb#20011 * github.com:scylladb/scylladb: test/sstable: remove useless variable from promoted_index_read() test/sstable: rewrite promoted_index_read() with async() test/sstable: unfuturize lambda invocation in test_using_reusable_sst*() test/sstable: rewrite wrong_range() with async() test/sstable: simplify not_find_key_composite_bucket0() under test_using_reusable_sst() test/sstable: rewrite full_index_search() with async() test/sstable: simplify find_key*(), all_in_place() under test_using_reusable_sst() test/sstable: rewrite (un)compressed_random_access_read() with async() test/sstable: simplify write_and_validate_sst() test/sstable: simplify check_toc_func() under async() test/sstable: simplify check_statistics_func() under async() test/sstable: simplify check_summary_func() under async() test/sstable: coroutinize check_component_integrity() test/sstable: rewrite write_sst_info() with async() test/sstable: simplify missing_summary_first_last_sane() test/sstable: coroutinize summary_query_fail() test/sstable: rewrite summary_query() with async() test/sstable: coroutinize (simple/composite)_index_read() test/sstable: rewrite index_read() with async() test/sstable: rewrite test_using_reusable_sst() with async() test/sstable: rewrite test_using_working_sst() with async()