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

Commit

Permalink
rgw/sfs: Refactor add_delete_marker_transact
Browse files Browse the repository at this point in the history 
Make added the return value. Make id an optional out parameter as it
is only used by unit tests.

Signed-off-by: Marcel Lauhoff <[email protected]>
  • Loading branch information
Marcel Lauhoff committed Nov 16, 2023
1 parent 53aa541 commit f45c7dc
Show file tree
Hide file tree
Showing 5 changed files with 41 additions and 49 deletions.
17 changes: 9 additions & 8 deletions src/rgw/driver/sfs/sqlite/sqlite_versioned_objects.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -331,11 +331,9 @@ 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 transaction = storage->transaction_guard();
Expand All @@ -356,26 +354,29 @@ 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;
}

std::optional<DBVersionedObject>
Expand Down
5 changes: 3 additions & 2 deletions src/rgw/driver/sfs/sqlite/sqlite_versioned_objects.h
View file
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
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
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
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

0 comments on commit f45c7dc

Please sign in to comment.