From 1c2087f503fae32a19a4c02b89e83a0fdd9c1d48 Mon Sep 17 00:00:00 2001 From: Marcel Lauhoff Date: Thu, 16 Nov 2023 17:28:01 +0100 Subject: [PATCH] rgw/sfs: Retry Util sqlite_modern_cpp support Support sqlite_modern_cpp custom exceptions as well as sqlite_orm system error exceptions. Mark legacy code paths with TODOs for later removal. Signed-off-by: Marcel Lauhoff --- src/rgw/driver/sfs/sqlite/retry.h | 28 +++++++-- src/test/rgw/sfs/test_rgw_sfs_retry.cc | 85 +++++++++++++++++++++++--- 2 files changed, 102 insertions(+), 11 deletions(-) diff --git a/src/rgw/driver/sfs/sqlite/retry.h b/src/rgw/driver/sfs/sqlite/retry.h index 07c0af100a25c..c6f7d9715847d 100644 --- a/src/rgw/driver/sfs/sqlite/retry.h +++ b/src/rgw/driver/sfs/sqlite/retry.h @@ -18,6 +18,7 @@ #include #include +#include "dbapi.h" #include "errors.h" #include "rgw_perf_counters.h" @@ -39,7 +40,7 @@ class RetrySQLiteBusy { const int m_max_retries{10}; bool m_successful{false}; int m_retries{0}; - std::error_code m_failed_error{}; + int m_failed_sqlite_error{}; public: RetrySQLiteBusy(Func&& fn) : m_fn(std::forward(fn)) {} @@ -60,11 +61,13 @@ class RetrySQLiteBusy { try { Return result = m_fn(); m_successful = true; - m_failed_error = std::error_code{}; + m_failed_sqlite_error = SQLITE_OK; m_retries = retry; return result; + // TODO(https://github.com/aquarist-labs/s3gw/issues/788) Remove + // sqlite_orm path } catch (const std::system_error& ex) { - m_failed_error = ex.code(); + m_failed_sqlite_error = ex.code().value(); if (critical_error(ex.code().value())) { ceph_abort_msgf( "Critical SQLite error %d. Shutting down.", ex.code().value() @@ -80,6 +83,23 @@ class RetrySQLiteBusy { if (perfcounter) { perfcounter->inc(l_rgw_sfs_sqlite_retry_retried_count, 1); } + } catch (const dbapi::sqlite::sqlite_exception& ex) { + m_failed_sqlite_error = ex.get_code(); + if (critical_error(ex.get_code())) { + ceph_abort_msgf( + "Critical SQLite error %d. Shutting down.", ex.get_code() + ); + } + if (!busy_error(ex.get_code())) { + // Rethrow, expect a higher layer to handle (e.g constraint + // violations), reply internal server error or shut us down + throw ex; + } + std::this_thread::sleep_for(10ms * retry); + m_retries = retry; + if (perfcounter) { + perfcounter->inc(l_rgw_sfs_sqlite_retry_retried_count, 1); + } } } m_successful = false; @@ -94,7 +114,7 @@ class RetrySQLiteBusy { bool successful() { return m_successful; }; /// failed_error returns the non-critical error code of the last /// failed attempt to run fn - std::error_code failed_error() { return m_failed_error; }; + int failed_error() { return m_failed_sqlite_error; }; /// retries returns the number of retries to failure or success int retries() { return m_retries; }; }; diff --git a/src/test/rgw/sfs/test_rgw_sfs_retry.cc b/src/test/rgw/sfs/test_rgw_sfs_retry.cc index bfd27f2982078..3420a943bbdda 100644 --- a/src/test/rgw/sfs/test_rgw_sfs_retry.cc +++ b/src/test/rgw/sfs/test_rgw_sfs_retry.cc @@ -5,6 +5,7 @@ #include #include "common/ceph_context.h" +#include "driver/sfs/sqlite/dbapi.h" #include "driver/sfs/sqlite/retry.h" #include "driver/sfs/sqlite/sqlite_orm.h" #include "gtest/gtest.h" @@ -26,9 +27,13 @@ class TestSFSRetrySQLite : public ::testing::Test { void TearDown() override {} }; -class TestSFSRetrySQLiteDeathTest : public TestSFSRetrySQLite {}; +// TODO(https://github.com/aquarist-labs/s3gw/issues/788) Remove +// TestSFSRetrySQLiteOrm tests -TEST_F(TestSFSRetrySQLite, retry_non_crit_till_failure) { +class TestSFSRetrySQLiteOrm : public TestSFSRetrySQLite {}; +class TestSFSRetrySQLiteOrmDeathTest : public TestSFSRetrySQLiteOrm {}; + +TEST_F(TestSFSRetrySQLiteOrm, retry_non_crit_till_failure) { auto exception = std::system_error{SQLITE_BUSY, sqlite_orm::get_sqlite_error_category()}; RetrySQLiteBusy uut([&]() { @@ -37,11 +42,11 @@ TEST_F(TestSFSRetrySQLite, retry_non_crit_till_failure) { }); EXPECT_EQ(uut.run(), std::nullopt); EXPECT_FALSE(uut.successful()); - EXPECT_EQ(uut.failed_error(), exception.code()); + EXPECT_EQ(uut.failed_error(), exception.code().value()); EXPECT_GT(uut.retries(), 0); } -TEST_F(TestSFSRetrySQLiteDeathTest, crit_aborts) { +TEST_F(TestSFSRetrySQLiteOrmDeathTest, crit_aborts) { GTEST_FLAG_SET(death_test_style, "threadsafe"); auto exception = std::system_error{ SQLITE_CORRUPT, sqlite_orm::get_sqlite_error_category() @@ -53,14 +58,14 @@ TEST_F(TestSFSRetrySQLiteDeathTest, crit_aborts) { ASSERT_DEATH(uut.run(), "Critical SQLite error"); } -TEST_F(TestSFSRetrySQLite, simple_return_succeeds_immediately) { +TEST_F(TestSFSRetrySQLiteOrm, simple_return_succeeds_immediately) { RetrySQLiteBusy uut([&]() { return 42; }); EXPECT_EQ(uut.run(), 42); EXPECT_TRUE(uut.successful()); EXPECT_EQ(uut.retries(), 0); } -TEST_F(TestSFSRetrySQLite, retry_second_time_success) { +TEST_F(TestSFSRetrySQLiteOrm, retry_second_time_success) { auto exception = std::system_error{SQLITE_BUSY, sqlite_orm::get_sqlite_error_category()}; bool first = true; @@ -74,6 +79,72 @@ TEST_F(TestSFSRetrySQLite, retry_second_time_success) { }); EXPECT_EQ(uut.run(), 23); EXPECT_TRUE(uut.successful()); - EXPECT_NE(uut.failed_error(), exception.code()); + EXPECT_NE(uut.failed_error(), exception.code().value()); + EXPECT_EQ(uut.retries(), 1); +} + +class TestSFSRetrySQLiteDeathTest : public TestSFSRetrySQLite {}; + +TEST_F(TestSFSRetrySQLite, retry_non_crit_till_failure) { + auto exception = rgw::sal::sfs::dbapi::sqlite::errors::busy(SQLITE_BUSY, ""); + RetrySQLiteBusy uut([&]() { + throw exception; + return 0; + }); + EXPECT_EQ(uut.run(), std::nullopt); + EXPECT_FALSE(uut.successful()); + EXPECT_EQ(uut.failed_error(), exception.get_code()); + EXPECT_GT(uut.retries(), 0); +} + +TEST_F(TestSFSRetrySQLite, retry_non_crit_extended_till_failure) { + auto exception = rgw::sal::sfs::dbapi::sqlite::errors::busy_snapshot( + SQLITE_BUSY_SNAPSHOT, "" + ); + RetrySQLiteBusy uut([&]() { + throw exception; + return 0; + }); + EXPECT_EQ(uut.run(), std::nullopt); + EXPECT_FALSE(uut.successful()); + EXPECT_EQ(uut.failed_error(), exception.get_code()); + EXPECT_GT(uut.retries(), 0); +} + +TEST_F(TestSFSRetrySQLiteDeathTest, crit_aborts) { + GTEST_FLAG_SET(death_test_style, "threadsafe"); + auto exception = std::system_error{ + SQLITE_CORRUPT, sqlite_orm::get_sqlite_error_category() + }; + RetrySQLiteBusy uut([&]() { + rgw::sal::sfs::dbapi::sqlite::errors::throw_sqlite_error(SQLITE_CORRUPT); + return 0; + }); + ASSERT_DEATH(uut.run(), "Critical SQLite error"); +} + +TEST_F(TestSFSRetrySQLite, simple_return_succeeds_immediately) { + RetrySQLiteBusy uut([&]() { return 42; }); + EXPECT_EQ(uut.run(), 42); + EXPECT_TRUE(uut.successful()); + EXPECT_EQ(uut.retries(), 0); +} + +TEST_F(TestSFSRetrySQLite, retry_second_time_success) { + auto exception = rgw::sal::sfs::dbapi::sqlite::sqlite_exception( + SQLITE_BUSY, "", "non critical error" + ); + bool first = true; + RetrySQLiteBusy uut([&]() { + if (first) { + first = false; + throw exception; + } else { + return 23; + } + }); + EXPECT_EQ(uut.run(), 23); + EXPECT_TRUE(uut.successful()); + EXPECT_NE(uut.failed_error(), exception.get_code()); EXPECT_EQ(uut.retries(), 1); }