-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add delete_many functionality to object store #360
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. Please address few comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. Couple more minor comments.
@@ -365,6 +367,9 @@ def _run_hash_and_merge( | |||
mutable_compaction_audit.set_telemetry_time_in_seconds( | |||
telemetry_this_round + previous_telemetry | |||
) | |||
if params.num_rounds > 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add an e2e functional test using a Test object store impl to test this behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see e2e functional test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an assertion to our multiple rounds test, it checks that delete_many
was called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I am looking for an assertion. Add an e2e test with multiple rounds that ensures object ends up clean after compaction.
@@ -327,4 +328,5 @@ def test_compact_partition_rebase_multiple_rounds_same_source_and_destination( | |||
if assert_compaction_audit: | |||
if not assert_compaction_audit(compactor_version, compaction_audit): | |||
assert False, "Compaction audit assertion failed" | |||
assert object_store_delete_many_spy.call_count, "Object store was never cleaned up!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just checks if delete_many was called. We want to test behavior i.e., the object store actually ends up with no data after the compaction.
This change adds the
delete_many
functionality to object stores. This is necessary to cleanup the object store between rounds of compaction.