Skip to content

feat(storage): add support for conditional idempotent operations #18834

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.

Sign up for GitHub

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

Merged
bajajneha27 merged 12 commits into from
Aug 22, 2022
Merged

feat(storage): add support for conditional idempotent operations #18834

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
4163740
feat(storage):add support for conditional idempotent operations
bajajneha27 Jul 7, 2022
1f4d5bc
add conditional idempotency to delete object
bajajneha27 Jul 7, 2022
3167984
add conditional idempotency to compose object
bajajneha27 Jul 7, 2022
556274f
add conditional idempotency to rewrite object
bajajneha27 Jul 7, 2022
80d85c5
address review comments
bajajneha27 Jul 20, 2022
036d55f
fix rubocop errors
bajajneha27 Jul 20, 2022
39d40f8
fix rubocop issue
bajajneha27 Jul 20, 2022
8281989
dummy commit for kokoro freshr run
bajajneha27 Jul 21, 2022
4a568cb
Allow users to override default behaviour for conditional retry opera…
bajajneha27 Aug 1, 2022
cec0ff2
fix rubocop
bajajneha27 Aug 1, 2022
9dd64f8
update doc to sow usage of options field and add options for all oper…
bajajneha27 Aug 8, 2022
9cad94e
fix samples test cases
bajajneha27 Aug 8, 2022
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
Prev Previous commit
Next Next commit
add conditional idempotency to rewrite object
  • Loading branch information
@bajajneha27
bajajneha27 committed Aug 8, 2022
commit 556274fdbe23056e2bc9db83d387715c31025b81
28 changes: 20 additions & 8 deletions google-cloud-storage/lib/google/cloud/storage/service.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,9 @@ def patch_bucket bucket_name,
bucket_gapi ||= Google::Apis::StorageV1::Bucket.new
bucket_gapi.acl = [] if predefined_acl
bucket_gapi.default_object_acl = [] if predefined_default_acl
is_idempotent = Retry.retry?({if_metageneration_match: if_metageneration_match})

is_idempotent = Retry.retry?(if_metageneration_match: if_metageneration_match)
options = is_idempotent ? {} : {retries: 0}

execute do
service.patch_bucket bucket_name,
Expand All @@ -141,7 +143,7 @@ def patch_bucket bucket_name,
if_metageneration_match: if_metageneration_match,
if_metageneration_not_match: if_metageneration_not_match,
user_project: user_project(user_project),
options: {is_idempotent: is_idempotent}
options: options
end
end

Expand Down Expand Up @@ -359,8 +361,8 @@ def insert_file bucket_name,
file_obj = Google::Apis::StorageV1::Object.new(**params)
content_type ||= mime_type_for(path || Pathname(source).to_path)

is_idempotent = Retry.retry?({if_generation_match: if_generation_match})
options = key_options(key).merge({is_idempotent: is_idempotent})
is_idempotent = Retry.retry?(if_generation_match: if_generation_match)
options = is_idempotent ? key_options(key) : key_options(key).merge(retries: 0)

execute do
service.insert_object bucket_name,
Expand Down Expand Up @@ -427,6 +429,10 @@ def rewrite_file source_bucket_name,
token: nil,
user_project: nil
key_options = rewrite_key_options source_key, destination_key

is_idempotent = Retry.retry?(if_generation_match: if_generation_match)
Copy link

@tritone tritone Jul 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this should be for the destination file, not the source-- not sure how this is distinguished in Ruby.

Copy link
Contributor Author

@bajajneha27 bajajneha27 Jul 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is for destination file and if_source_generation_match is for source file.

options = is_idempotent ? key_options : key_options.merge(retries: 0)

execute do
service.rewrite_object source_bucket_name,
source_file_path,
Expand All @@ -446,7 +452,7 @@ def rewrite_file source_bucket_name,
if_source_metageneration_not_match: if_source_metageneration_not_match,
rewrite_token: token,
user_project: user_project(user_project),
options: key_options
options: options
end
end

Expand All @@ -467,7 +473,7 @@ def compose_file bucket_name,
compose_req = Google::Apis::StorageV1::ComposeRequest.new source_objects: source_objects,
destination: destination_gapi
is_idempotent = Retry.retry?(if_generation_match: if_generation_match)
options = key_options(key).merge({is_idempotent: is_idempotent})
options = is_idempotent ? key_options(key) : key_options(key).merge(retries: 0)

execute do
service.compose_object bucket_name,
Expand Down Expand Up @@ -517,6 +523,10 @@ def patch_file bucket_name,
predefined_acl: nil,
user_project: nil
file_gapi ||= Google::Apis::StorageV1::Object.new

is_idempotent = Retry.retry?(if_metageneration_match: if_metageneration_match)
options = is_idempotent ? {} : {retries: 0}

execute do
service.patch_object bucket_name,
file_path,
Expand All @@ -527,7 +537,8 @@ def patch_file bucket_name,
if_metageneration_match: if_metageneration_match,
if_metageneration_not_match: if_metageneration_not_match,
predefined_acl: predefined_acl,
user_project: user_project(user_project)
user_project: user_project(user_project),
options: options
end
end

Expand All @@ -542,6 +553,7 @@ def delete_file bucket_name,
if_metageneration_not_match: nil,
user_project: nil
is_idempotent = Retry.retry?(generation: generation, if_generation_match: if_generation_match)
options = is_idempotent ? {} : {retries: 0}

execute do
service.delete_object bucket_name, file_path,
Expand All @@ -551,7 +563,7 @@ def delete_file bucket_name,
if_metageneration_match: if_metageneration_match,
if_metageneration_not_match: if_metageneration_not_match,
user_project: user_project(user_project),
options: {is_idempotent: is_idempotent}
options: options
end
end

Expand Down
4 changes: 2 additions & 2 deletions google-cloud-storage/test/google/cloud/storage/bucket_acl_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@
mock.expect :get_bucket, bucket_gapi, [bucket_name], **get_bucket_args(user_project: "test")
mock.expect :patch_bucket,
Google::Apis::StorageV1::Bucket.from_json(random_bucket_hash(name: bucket_name).to_json),
[bucket_name, Google::Apis::StorageV1::Bucket.new(acl: [])], **patch_bucket_args(predefined_acl: "authenticatedRead", user_project: "test")
[bucket_name, Google::Apis::StorageV1::Bucket.new(acl: [])], **patch_bucket_args(predefined_acl: "authenticatedRead", user_project: "test", options: {retries: 0})

storage.service.mocked_service = mock

Expand Down Expand Up @@ -423,7 +423,7 @@ def predefined_acl_update acl_role
mock = Minitest::Mock.new
mock.expect :patch_bucket,
Google::Apis::StorageV1::Bucket.from_json(random_bucket_hash(name: bucket.name).to_json),
[bucket_name, Google::Apis::StorageV1::Bucket.new(acl: [])], **patch_bucket_args(predefined_acl: acl_role)
[bucket_name, Google::Apis::StorageV1::Bucket.new(acl: [])], **patch_bucket_args(predefined_acl: acl_role, options: {retries: 0})

storage.service.mocked_service = mock

Expand Down
24 changes: 12 additions & 12 deletions google-cloud-storage/test/google/cloud/storage/bucket_compose_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
it "can compose a new file with string sources" do
mock = Minitest::Mock.new
req = compose_request [file_name, file_2_name]
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args(options: {retries: 0})

bucket.service.mocked_service = mock

Expand All @@ -67,7 +67,7 @@
it "can compose a new file with File sources" do
mock = Minitest::Mock.new
req = compose_request [file_gapi, file_2_gapi]
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args(options: {retries: 0})

bucket.service.mocked_service = mock

Expand All @@ -84,7 +84,7 @@

mock = Minitest::Mock.new
req = compose_request [file_gapi, file_2_gapi]
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args(options: {retries: 0})

bucket.service.mocked_service = mock

Expand All @@ -100,7 +100,7 @@
req = compose_request [file_gapi, file_2_gapi], if_source_generation_match: [generation, generation_2]
mock.expect :compose_object,
file_3_gapi,
[bucket.name, file_3_name, req], **compose_object_args
[bucket.name, file_3_name, req], **compose_object_args(options: {retries: 0})
bucket.service.mocked_service = mock

new_file = bucket.compose [file, file_2], file_3_name, if_source_generation_match: [generation, generation_2]
Expand All @@ -119,7 +119,7 @@
it "can compose a new file with predefined ACL" do
mock = Minitest::Mock.new
req = compose_request [file_gapi, file_2_gapi]
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args(destination_predefined_acl: "private")
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args(destination_predefined_acl: "private", options: {retries: 0})

bucket.service.mocked_service = mock

Expand All @@ -133,7 +133,7 @@
it "can compose a new file with ACL alias" do
mock = Minitest::Mock.new
req = compose_request [file_gapi, file_2_gapi]
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args(destination_predefined_acl: "publicRead")
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args(destination_predefined_acl: "publicRead", options: {retries: 0})

bucket.service.mocked_service = mock

Expand All @@ -147,7 +147,7 @@
it "can compose a new file with if_generation_match" do
mock = Minitest::Mock.new
req = compose_request [file_gapi, file_2_gapi]
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args(if_generation_match: generation, is_idempotent: true)
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args(if_generation_match: generation)

bucket.service.mocked_service = mock

Expand All @@ -161,7 +161,7 @@
it "can compose a new file with if_metageneration_match" do
mock = Minitest::Mock.new
req = compose_request [file_gapi, file_2_gapi]
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args(if_metageneration_match: metageneration)
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args(if_metageneration_match: metageneration, options: {retries: 0})

bucket.service.mocked_service = mock

Expand All @@ -175,7 +175,7 @@
it "can compose a new file with user_project set to true" do
mock = Minitest::Mock.new
req = compose_request [file_gapi, file_2_gapi]
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args(user_project: "test")
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args(user_project: "test", options: {retries: 0})

file.service.mocked_service = mock

Expand All @@ -190,7 +190,7 @@
it "can compose a new file with customer-supplied encryption key" do
mock = Minitest::Mock.new
req = compose_request [file_gapi, file_2_gapi]
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args(options: key_options)
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args(options: key_options.merge(retries: 0))

file.service.mocked_service = mock

Expand All @@ -213,7 +213,7 @@
storage_class: "NEARLINE"
)
req = compose_request [file_gapi, file_2_gapi], update_file_gapi
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args(options: {retries: 0})

bucket.service.mocked_service = mock

Expand Down Expand Up @@ -246,7 +246,7 @@
storage_class: "NEARLINE"
)
req = compose_request [file_gapi, file_2_gapi], update_file_gapi
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args(user_project: "test")
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args(user_project: "test", options: {retries: 0})

bucket.service.mocked_service = mock

Expand Down
2 changes: 1 addition & 1 deletion google-cloud-storage/test/google/cloud/storage/bucket_default_acl_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ def predefined_default_acl_update acl_role
mock = Minitest::Mock.new
mock.expect :patch_bucket,
Google::Apis::StorageV1::Bucket.from_json(random_bucket_hash(name: bucket.name).to_json),
[bucket_name, Google::Apis::StorageV1::Bucket.new(default_object_acl: [])], **patch_bucket_args(predefined_default_object_acl: acl_role)
[bucket_name, Google::Apis::StorageV1::Bucket.new(default_object_acl: [])], **patch_bucket_args(predefined_default_object_acl: acl_role, options: {retries: 0})

storage.service.mocked_service = mock

Expand Down
8 changes: 4 additions & 4 deletions google-cloud-storage/test/google/cloud/storage/bucket_encryption_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@

mock = Minitest::Mock.new
mock.expect :insert_object, create_file_gapi(bucket.name, new_file_name),
[bucket.name, empty_file_gapi], **insert_object_args(name: new_file_name, upload_source: tmpfile, options: key_options)
[bucket.name, empty_file_gapi], **insert_object_args(name: new_file_name, upload_source: tmpfile, options: key_options.merge(retries: 0))

bucket.service.mocked_service = mock

Expand Down Expand Up @@ -75,7 +75,7 @@
it "gets and sets its encryption config" do
mock = Minitest::Mock.new
patch_bucket_gapi = Google::Apis::StorageV1::Bucket.new encryption: encryption_gapi(kms_key)
mock.expect :patch_bucket, patch_bucket_gapi, [bucket_name, patch_bucket_gapi], **patch_bucket_args
mock.expect :patch_bucket, patch_bucket_gapi, [bucket_name, patch_bucket_gapi], **patch_bucket_args(options: {retries: 0})

bucket.service.mocked_service = mock

Expand All @@ -92,7 +92,7 @@
bucket_with_key = Google::Cloud::Storage::Bucket.from_gapi bucket_gapi_with_key, storage.service
patch_bucket_gapi = Google::Apis::StorageV1::Bucket.new encryption: encryption_gapi(nil)
mock = Minitest::Mock.new
mock.expect :patch_bucket, bucket_gapi, [bucket_name, patch_bucket_gapi], **patch_bucket_args
mock.expect :patch_bucket, bucket_gapi, [bucket_name, patch_bucket_gapi], **patch_bucket_args(options: {retries: 0})

bucket_with_key.service.mocked_service = mock

Expand All @@ -111,7 +111,7 @@

mock = Minitest::Mock.new
mock.expect :insert_object, create_file_gapi(bucket.name, new_file_name),
[bucket.name, empty_file_gapi], **insert_object_args(name: new_file_name, upload_source: tmpfile, kms_key_name: kms_key)
[bucket.name, empty_file_gapi], **insert_object_args(name: new_file_name, upload_source: tmpfile, kms_key_name: kms_key, options: {retries: 0})

bucket.service.mocked_service = mock

Expand Down
4 changes: 2 additions & 2 deletions google-cloud-storage/test/google/cloud/storage/bucket_lock_retention_policy_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
mock = Minitest::Mock.new
patch_retention_policy_gapi = Google::Apis::StorageV1::Bucket::RetentionPolicy.new retention_period: bucket_retention_period
patch_bucket_gapi = Google::Apis::StorageV1::Bucket.new retention_policy: patch_retention_policy_gapi
mock.expect :patch_bucket, bucket_with_retention_policy_gapi, [bucket_name, patch_bucket_gapi], **patch_bucket_args
mock.expect :patch_bucket, bucket_with_retention_policy_gapi, [bucket_name, patch_bucket_gapi], **patch_bucket_args(options: {retries: 0})
bucket.service.mocked_service = mock

_(bucket.retention_period).must_be :nil?
Expand All @@ -67,7 +67,7 @@
it "updates its default_event_based_hold" do
mock = Minitest::Mock.new
patch_bucket_gapi = Google::Apis::StorageV1::Bucket.new default_event_based_hold: true
mock.expect :patch_bucket, bucket_with_retention_policy_gapi, [bucket_name, patch_bucket_gapi], **patch_bucket_args
mock.expect :patch_bucket, bucket_with_retention_policy_gapi, [bucket_name, patch_bucket_gapi], **patch_bucket_args(options: {retries: 0})
bucket.service.mocked_service = mock

_(bucket.default_event_based_hold?).must_equal false
Expand Down
8 changes: 4 additions & 4 deletions google-cloud-storage/test/google/cloud/storage/bucket_public_access_prevention_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@
it "updates its public_access_prevention" do
mock = Minitest::Mock.new
mock.expect :patch_bucket, resp_bucket_gapi(bucket_hash, public_access_prevention: "inherited"),
[bucket_name, patch_bucket_gapi(public_access_prevention: "inherited")], **patch_bucket_args
[bucket_name, patch_bucket_gapi(public_access_prevention: "inherited")], **patch_bucket_args(options: {retries: 0})
mock.expect :patch_bucket, resp_bucket_gapi(bucket_hash, public_access_prevention: "enforced"),
[bucket_name, patch_bucket_gapi(public_access_prevention: "enforced")], **patch_bucket_args
[bucket_name, patch_bucket_gapi(public_access_prevention: "enforced")], **patch_bucket_args(options: {retries: 0})
bucket.service.mocked_service = mock

_(bucket.public_access_prevention).must_be :nil?
Expand All @@ -63,9 +63,9 @@
it "updates its public_access_prevention with user_project set to true" do
mock = Minitest::Mock.new
mock.expect :patch_bucket, resp_bucket_gapi(bucket_hash, public_access_prevention: "inherited"),
[bucket_name, patch_bucket_gapi(public_access_prevention: "inherited")], **patch_bucket_args(user_project: "test")
[bucket_name, patch_bucket_gapi(public_access_prevention: "inherited")], **patch_bucket_args(user_project: "test", options: {retries: 0})
mock.expect :patch_bucket, resp_bucket_gapi(bucket_hash, public_access_prevention: "enforced"),
[bucket_name, patch_bucket_gapi(public_access_prevention: "enforced")], **patch_bucket_args(user_project: "test")
[bucket_name, patch_bucket_gapi(public_access_prevention: "enforced")], **patch_bucket_args(user_project: "test", options: {retries: 0})

bucket_user_project.service.mocked_service = mock

Expand Down
4 changes: 2 additions & 2 deletions google-cloud-storage/test/google/cloud/storage/bucket_rpo_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
it "updates its rpo" do
mock = Minitest::Mock.new
mock.expect :patch_bucket, resp_bucket_gapi(dual_region_bucket_hash, rpo: "ASYNC_TURBO"),
[dual_region_bucket_name, patch_bucket_gapi(rpo: "ASYNC_TURBO")], **patch_bucket_args
[dual_region_bucket_name, patch_bucket_gapi(rpo: "ASYNC_TURBO")], **patch_bucket_args(options: {retries: 0})

dual_region_bucket.service.mocked_service = mock

Expand All @@ -49,7 +49,7 @@
it "fails to update its rpo because it's multi-region" do
mock = Minitest::Mock.new
mock.expect :patch_bucket, resp_bucket_gapi(bucket_hash, rpo: "DEFAULT"),
[bucket_name, patch_bucket_gapi(rpo: "ASYNC_TURBO")], **patch_bucket_args
[bucket_name, patch_bucket_gapi(rpo: "ASYNC_TURBO")], **patch_bucket_args(options: {retries: 0})

bucket.service.mocked_service = mock

Expand Down
Loading