Skip to content
This repository has been archived by the owner on Jan 3, 2024. It is now read-only.

Retry add_delete_marker_transact #248

Merged
merged 2 commits into from
Nov 28, 2023
Merged

Retry add_delete_marker_transact #248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f45c7dc
rgw/sfs: Refactor add_delete_marker_transact
Nov 16, 2023
bbd7386
rgw/sfs: Retry delete_version_and_get_previous_transact
Nov 16, 2023
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
29 changes: 14 additions & 15 deletions src/rgw/driver/sfs/sqlite/sqlite_versioned_objects.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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>
Expand Down
5 changes: 3 additions & 2 deletions src/rgw/driver/sfs/sqlite/sqlite_versioned_objects.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
5 changes: 2 additions & 3 deletions src/rgw/driver/sfs/types.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,9 +431,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;
Expand Down
8 changes: 3 additions & 5 deletions src/test/rgw/sfs/test_rgw_sfs_sqlite_buckets.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Expand Down
55 changes: 24 additions & 31 deletions src/test/rgw/sfs/test_rgw_sfs_sqlite_versioned_objects.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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());
Expand All @@ -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());
Expand All @@ -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(
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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(
Expand Down
Loading