Merge pull request #248 from irq0/pr/retry-update

src/rgw/driver/sfs/sqlite/sqlite_versioned_objects.cc

@@ -331,13 +331,11 @@ SQLiteVersionedObjects::delete_version_and_get_previous_transact(
   }
 }
 
-uint SQLiteVersionedObjects::add_delete_marker_transact(
-    const uuid_d& object_id, const std::string& delete_marker_id, bool& added
+bool SQLiteVersionedObjects::add_delete_marker_transact(
+    const uuid_d& object_id, const std::string& delete_marker_id, uint* out_id
 ) const {
-  uint ret_id{0};
-  added = false;
-  try {
-    auto storage = conn->get_storage();
+  auto storage = conn->get_storage();
+  RetrySQLiteBusy<bool> retry([&]() {
     auto transaction = storage->transaction_guard();
     auto last_version_select = storage->get_all<DBVersionedObject>(
         where(
@@ -356,26 +354,27 @@ uint SQLiteVersionedObjects::add_delete_marker_transact(
       if ((last_version.object_state == ObjectState::COMMITTED ||
            last_version.object_state == ObjectState::OPEN) &&
           last_version.version_type == VersionType::REGULAR) {
-        auto now = ceph::real_clock::now();
+        const auto now = ceph::real_clock::now();
         last_version.version_type = VersionType::DELETE_MARKER;
         last_version.object_state = ObjectState::COMMITTED;
         last_version.commit_time = now;
         last_version.delete_time = now;
         last_version.mtime = now;
         last_version.version_id = delete_marker_id;
-        ret_id = storage->insert(last_version);
-        added = true;
+        const uint ret_id = storage->insert(last_version);
+        if (out_id) {
+          *out_id = ret_id;
+        }
         // only commit if the delete maker was indeed inserted.
         // the rest of calls in this transaction are read operations
         transaction.commit();
+        return true;
       }
     }
-  } catch (const std::system_error& e) {
-    // throw exception (will be caught later in the sfs logic)
-    // TODO revisit this when error handling is defined
-    throw(e);
-  }
-  return ret_id;
+    return false;
+  });
+  const auto result = retry.run();
+  return result.has_value() ? result.value() : false;
 }
 
 std::optional<DBVersionedObject>

src/rgw/driver/sfs/sqlite/sqlite_versioned_objects.h

@@ -72,8 +72,9 @@ class SQLiteVersionedObjects {
       const std::string& version_id
   ) const;
 
-  uint add_delete_marker_transact(
-      const uuid_d& object_id, const std::string& delete_marker_id, bool& added
+  bool add_delete_marker_transact(
+      const uuid_d& object_id, const std::string& delete_marker_id,
+      uint* out_id = nullptr
   ) const;
 
   std::optional<DBDeletedObjectItems> remove_deleted_versions_transact(

src/rgw/driver/sfs/types.cc

@@ -434,9 +434,8 @@ std::string Bucket::_add_delete_marker(
     const sqlite::SQLiteVersionedObjects& db_versioned_objs
 ) const {
   std::string delete_marker_id = generate_new_version_id(store->ceph_context());
-  bool added;
-  db_versioned_objs.add_delete_marker_transact(
-      obj.path.get_uuid(), delete_marker_id, added
+  const bool added = db_versioned_objs.add_delete_marker_transact(
+      obj.path.get_uuid(), delete_marker_id
   );
   if (added) {
     return delete_marker_id;

src/test/rgw/sfs/test_rgw_sfs_sqlite_buckets.cc

@@ -659,11 +659,9 @@ TEST_F(TestSFSSQLiteBuckets, TestBucketEmpty) {
   EXPECT_FALSE(db_buckets->bucket_empty("bucket1_id"));
 
   // add a delete marker
-  bool delete_marker_added = false;
-  db_versions->add_delete_marker_transact(
-      version1->object_id, "delete_maker_1", delete_marker_added
-  );
-  ASSERT_TRUE(delete_marker_added);
+  ASSERT_TRUE(db_versions->add_delete_marker_transact(
+      version1->object_id, "delete_maker_1"
+  ));
 
   // bucket is still not empty
   EXPECT_FALSE(db_buckets->bucket_empty("bucket1_id"));

src/test/rgw/sfs/test_rgw_sfs_sqlite_versioned_objects.cc

@@ -1307,11 +1307,10 @@ TEST_F(TestSFSSQLiteVersionedObjects, TestAddDeleteMarker) {
   auto delete_marker_id = "delete_marker_id";
   uuid_d uuid;
   uuid.parse(TEST_OBJECT_ID.c_str());
-  bool added;
-  auto id = db_versioned_objects->add_delete_marker_transact(
-      uuid, delete_marker_id, added
-  );
-  EXPECT_TRUE(added);
+  uint id;
+  EXPECT_TRUE(db_versioned_objects->add_delete_marker_transact(
+      uuid, delete_marker_id, &id
+  ));
   EXPECT_EQ(4, id);
   auto delete_marker = db_versioned_objects->get_versioned_object(4);
   ASSERT_TRUE(delete_marker.has_value());
@@ -1324,10 +1323,11 @@ TEST_F(TestSFSSQLiteVersionedObjects, TestAddDeleteMarker) {
 
   // add another delete marker (should not add it because the marker already
   // exists)
-  id = db_versioned_objects->add_delete_marker_transact(
-      uuid, delete_marker_id, added
+  EXPECT_FALSE(
+      id = db_versioned_objects->add_delete_marker_transact(
+          uuid, delete_marker_id, &id
+      )
   );
-  EXPECT_FALSE(added);
   EXPECT_EQ(0, id);
   auto last_version = db_versioned_objects->get_versioned_object(5);
   ASSERT_FALSE(last_version.has_value());
@@ -1348,10 +1348,9 @@ TEST_F(TestSFSSQLiteVersionedObjects, TestAddDeleteMarker) {
   db_versioned_objects->store_versioned_object(*read_version);
 
   // try to create the delete marker (we still have 1 alive version)
-  id = db_versioned_objects->add_delete_marker_transact(
-      uuid, delete_marker_id, added
-  );
-  EXPECT_TRUE(added);
+  EXPECT_TRUE(db_versioned_objects->add_delete_marker_transact(
+      uuid, delete_marker_id, &id
+  ));
   EXPECT_EQ(5, id);
   delete_marker = db_versioned_objects->get_versioned_object(5);
   ASSERT_TRUE(delete_marker.has_value());
@@ -1373,11 +1372,9 @@ TEST_F(TestSFSSQLiteVersionedObjects, TestAddDeleteMarker) {
 
   // add another delete marker (should not add it because all the versions of
   // the object are deleted)
-  id = db_versioned_objects->add_delete_marker_transact(
-      uuid, delete_marker_id, added
-  );
-  EXPECT_FALSE(added);
-  EXPECT_EQ(0, id);
+  EXPECT_FALSE(db_versioned_objects->add_delete_marker_transact(
+      uuid, delete_marker_id, &id
+  ));
 }
 
 void insertNCommittedVersionsIncrementingSize(
@@ -1497,11 +1494,9 @@ TEST_F(TestSFSSQLiteVersionedObjects, TestRemovedDeletedVersions) {
   uuid_object_1.parse(TEST_OBJECT_ID.c_str());
 
   // add a delete marker on the first object
-  bool delete_marker_added = false;
-  db_versioned_objects->add_delete_marker_transact(
-      uuid_object_1, "delete_marker_1", delete_marker_added
-  );
-  EXPECT_TRUE(delete_marker_added);
+  EXPECT_TRUE(db_versioned_objects->add_delete_marker_transact(
+      uuid_object_1, "delete_marker_1"
+  ));
 
   // call to delete again, nothing should be deleted
   deleted_objs_optional =
@@ -1756,11 +1751,10 @@ TEST_F(TestSFSSQLiteVersionedObjects, TestDeleteMarkerNotAlwaysOnTop) {
   uuid_d uuid;
   uuid.parse(TEST_OBJECT_ID.c_str());
 
-  bool added;
-  auto id = db_versioned_objects->add_delete_marker_transact(
-      uuid, delete_marker_id, added
-  );
-  EXPECT_TRUE(added);
+  uint id;
+  EXPECT_TRUE(db_versioned_objects->add_delete_marker_transact(
+      uuid, delete_marker_id, &id
+  ));
   EXPECT_EQ(4, id);
   auto delete_marker = db_versioned_objects->get_versioned_object(4);
   ASSERT_TRUE(delete_marker.has_value());
@@ -1792,10 +1786,9 @@ TEST_F(TestSFSSQLiteVersionedObjects, TestDeleteMarkerNotAlwaysOnTop) {
 
   // another delete marker
   delete_marker_id = "delete_marker_id_2";
-  id = db_versioned_objects->add_delete_marker_transact(
-      uuid, delete_marker_id, added
-  );
-  EXPECT_TRUE(added);
+  EXPECT_TRUE(db_versioned_objects->add_delete_marker_transact(
+      uuid, delete_marker_id, &id
+  ));
 
   // check that the new delete marker is the last version now
   last_version = db_versioned_objects->get_committed_versioned_object(