Remove Object Copy Cache #124

irq0 · 2023-03-01T11:08:48Z

Removes the object copy cache to reduce mutexes in the hot path and to enable us to replace linear time object list filters with (indexed) SQL queries.

Split into two parts: 1. Refactoring to encapsulate Object and access to the objects cache better. 2. Remove the cache.

No new tests. Changes already covered by existing tests.

Issue https://github.com/aquarist-labs/s3gw/issues/214
Prepares code base for aquarist-labs/s3gw-tools#203

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

giubacc · 2023-03-03T10:36:23Z

Extremely good work.
I think this is a fundamental refactoring improving greatly code-quality/performances.
I will give a deeper look next week.

giubacc · 2023-03-06T08:56:56Z

@irq0
It seems reasonably all good; I would like this PR to include the tests from legal holds PR because that contains a specific patch to certain methods modified in this refactoring.
Do you you think it feasible to merge that before this ? But you will have to rebase.

jecluis

generally looks good, but there are a few things that caught my attention. I'll need to do a second pass tomorrow, after reading some code, because I have the nagging feeling we may succumb to races.

src/rgw/driver/sfs/types.cc

src/rgw/driver/sfs/types.h

src/rgw/driver/sfs/types.cc

jecluis · 2023-03-06T18:32:36Z

src/rgw/driver/sfs/types.cc

@@ -258,52 +323,75 @@ void MultipartUpload::abort(const DoutPrefixProvider *dpp) {
  objref.reset();
 }

-bool Bucket::want_new_version(const rgw_obj_key& key, ObjectRef obj) {
-  return !(key.instance.empty() || key.instance == obj->instance);
+bool Bucket::want_specific_version(const rgw_obj_key& key) {


the comment on the previous commit goes for this one, I would rather have s/want/wants/.

👍 agreed. Replacing with local variable as well, the method doesn't make much sense outside get_or_create

src/rgw/driver/sfs/types.cc

0xavi0

Looks generally okay to me but I think I'd need another round of review.
The only thing I that I would request to be changed so far is calling std::move on raw pointers.
The "constructor helpers" thingy is more a personal taste thing.

0xavi0 · 2023-03-13T14:31:58Z

src/rgw/driver/sfs/types.cc

+
+Object* Object::create_for_multipart(const std::string& name) {
+  Object* result = new Object(name, UUIDPath::create().get_uuid());
+  return result;


Are all these really needed?

I see them as constructor helpers.

create_for_testing and create_for_multipart are the same. (at least for now, it they diverge at some point happy to have 2 different methods)

What about a single create_from_name? (or even a constructor with a single parameter which is the name)

And the create_for_immediate_deletion seems a bit weird to me.
If the object is immediately deleted I guess we don't need to set the deleted attribute to false.
I've tested this already removing the result->deleted = true; statement and as expected the GC tests pass.

I think calling the already existing Object(const std::string& _name, const uuid_d& _uuid) would be enough and we can save having that method around. (less code to be maintained)

I dunno, maybe it is a personal thing... but... to me...
create_from_obj_key would be just:

Object(const rgw_obj_key & key)

The one with only the name would be:

Object(const std::string& name)

and I would maybe just keep the helper for the query thing.

The named constructors try to make the more clear what Object is created for. We sometimes use it to retrieve data, sometimes update data, sometimes create a new object. I'd like to remove some of the use cases from Object over time. The named constructors are a first step in that direction. They do feel like overkill right now, but will make it easier to remove dependencies from Object IMHO.

0xavi0 · 2023-03-13T14:32:19Z

src/rgw/driver/sfs/object.cc

+      objref.reset(std::move(sfs::Object::create_for_query(get_name(), uuid, deleted,
+                                                           db_version->id)));


All the create_for_* functions return a raw pointer.
Why calling std::move on a raw pointer? I think it adds confusion to the reader.
It would make sense if create_for_* functions were retuning a std::unique_ptr...

good catch. will remove

github-actions · 2023-03-21T09:08:56Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

Improve Object and Bucket encapsulation to make it easy to remove the objects cache and mutex. Add specific named constructors (create_..) to Object that replace construction + metadata_init calls. Make Object create methods return pointers to allow callers to decided how to handle ownership. Without the object cache, we no longer need shared pointers of Object at all places. Separate Object data / metadata deletion (delete_object_data, delete_object_metadata) Add get, set, update methods for Object attrs and meta. Make both private fields. Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>

Remove objects cache. Replace with direct Object creation from our SQLite database. This changes semantics of Objects retrieved from Bucket::get_or_create. Previously they pointed to the _same_ in-memory, database object representation. Now, subsequent calls return different Objects created fresh from the database each time. Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>

Use docstrings, add issue reference. Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>

Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>

Move the 'from' before 'database' for better readability. Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>

Replace method with const bool wants_specific_version. Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>

irq0 · 2023-03-22T09:28:29Z

pr-triage failure: https://github.com/aquarist-labs/s3gw/issues/404

0xavi0 · 2023-03-23T07:37:16Z

We could, maybe, use this PR to remove (or adapt if we think it is useful for us) that pr-triage GH job...
Changes look good to me, but I guess we should address that CI issue first

jecluis · 2023-03-23T07:48:09Z

We could, maybe, use this PR to remove (or adapt if we think it is useful for us) that pr-triage GH job... Changes look good to me, but I guess we should address that CI issue first

that should be a different PR I think. This one can be merged ignoring that check. But I agree that should be fixed before merging this one.

stale

irq0 · 2023-03-23T08:33:05Z

local test run rebased on #122 successful

jecluis · 2023-03-23T08:41:46Z

note for the future: failed check has been dropped from this repository, but I was unable to find a way to rerun the workflows while dropping that specific check 🤷

github-actions bot added rgw tests labels Mar 1, 2023

irq0 force-pushed the pr/remove-types-objects branch 2 times, most recently from 143d1fc to c941dea Compare March 2, 2023 12:35

irq0 self-assigned this Mar 2, 2023

irq0 added this to the v0.13.0 milestone Mar 2, 2023

irq0 mentioned this pull request Mar 2, 2023

sfs/writer: Error handling and recovery #119

Merged

11 tasks

irq0 force-pushed the pr/remove-types-objects branch from c941dea to 79c6645 Compare March 2, 2023 15:29

irq0 marked this pull request as ready for review March 2, 2023 18:36

irq0 requested review from 0xavi0, jecluis and giubacc and removed request for 0xavi0 and jecluis March 2, 2023 18:37

jecluis suggested changes Mar 6, 2023

View reviewed changes

jhmarina mentioned this pull request Mar 7, 2023

Performance improvements (Epic) s3gw-tech/s3gw#214

Closed

irq0 mentioned this pull request Mar 7, 2023

Transact object create + object version create s3gw-tech/s3gw#378

Closed

irq0 force-pushed the pr/remove-types-objects branch 2 times, most recently from 0c6678e to 089ff0b Compare March 7, 2023 17:55

0xavi0 previously requested changes Mar 13, 2023

View reviewed changes

github-actions bot added the needs-rebase Changes need a rebase label Mar 21, 2023

Marcel Lauhoff added 2 commits March 21, 2023 14:56

irq0 force-pushed the pr/remove-types-objects branch from 089ff0b to e14e9c4 Compare March 21, 2023 14:00

github-actions bot added the api-change label Mar 21, 2023

github-actions bot added pybind rbd rook telemetry labels Mar 21, 2023

irq0 changed the base branch from s3gw-2023-03-old to s3gw March 21, 2023 16:09

irq0 removed the needs-rebase Changes need a rebase label Mar 21, 2023

irq0 marked this pull request as draft March 21, 2023 16:22

irq0 marked this pull request as ready for review March 21, 2023 16:22

irq0 force-pushed the pr/remove-types-objects branch from e14e9c4 to eef95bb Compare March 21, 2023 16:32

Marcel Lauhoff added 4 commits March 21, 2023 17:40

rgw/sfs: Improve types.{h,cc} comments

acc37f1

Use docstrings, add issue reference. Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>

rgw/sfs: Remove unused Bucket::get_unmutexed

3f0a29e

Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>

rgw/sfs: Rename Object::try_create... methods

6f5e545

Move the 'from' before 'database' for better readability. Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>

rgw/sfs: Pull want_specific_version into get_or_create

162e12d

Replace method with const bool wants_specific_version. Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>

irq0 force-pushed the pr/remove-types-objects branch from eef95bb to 162e12d Compare March 21, 2023 16:41

irq0 modified the milestones: v0.13.0, v0.14.0 Mar 21, 2023

irq0 removed the dashboard label Mar 21, 2023

irq0 requested review from jecluis and 0xavi0 March 22, 2023 09:25

irq0 mentioned this pull request Mar 22, 2023

Remove WRITING state #135

Merged

11 tasks

jecluis approved these changes Mar 23, 2023

View reviewed changes

irq0 merged commit 256ad80 into aquarist-labs:s3gw Mar 23, 2023

irq0 deleted the pr/remove-types-objects branch March 23, 2023 08:40

irq0 mentioned this pull request Apr 6, 2023

rgw/sfs: linear performance degradation with the number of objects in buckets s3gw-tech/s3gw#203

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Object Copy Cache #124

Remove Object Copy Cache #124

irq0 commented Mar 1, 2023

giubacc commented Mar 3, 2023

giubacc commented Mar 6, 2023

jecluis left a comment

jecluis Mar 6, 2023

irq0 Mar 7, 2023

0xavi0 left a comment

0xavi0 Mar 13, 2023

irq0 Mar 21, 2023

0xavi0 Mar 13, 2023

irq0 Mar 21, 2023

github-actions bot commented Mar 21, 2023

irq0 commented Mar 22, 2023

0xavi0 commented Mar 23, 2023

jecluis commented Mar 23, 2023

irq0 commented Mar 23, 2023

jecluis commented Mar 23, 2023

		objref.reset(std::move(sfs::Object::create_for_query(get_name(), uuid, deleted,
		db_version->id)));

Remove Object Copy Cache #124

Remove Object Copy Cache #124

Conversation

irq0 commented Mar 1, 2023

Checklist

giubacc commented Mar 3, 2023

giubacc commented Mar 6, 2023

jecluis left a comment

Choose a reason for hiding this comment

jecluis Mar 6, 2023

Choose a reason for hiding this comment

irq0 Mar 7, 2023

Choose a reason for hiding this comment

0xavi0 left a comment

Choose a reason for hiding this comment

0xavi0 Mar 13, 2023

Choose a reason for hiding this comment

irq0 Mar 21, 2023

Choose a reason for hiding this comment

0xavi0 Mar 13, 2023

Choose a reason for hiding this comment

irq0 Mar 21, 2023

Choose a reason for hiding this comment

github-actions bot commented Mar 21, 2023

irq0 commented Mar 22, 2023

0xavi0 commented Mar 23, 2023

jecluis commented Mar 23, 2023

irq0 commented Mar 23, 2023

jecluis commented Mar 23, 2023