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 all commits
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
30 changes: 30 additions & 0 deletions google-cloud-storage/OVERVIEW.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,6 +563,36 @@ require "google/cloud/storage"
storage = Google::Cloud::Storage.new retries: 10, timeout: 120
```

The library by default retries all API requests which are always idempotent on a
"transient" error.

For API requests which are idempotent only if the some conditions are satisfied
(For ex. a file has the same "generation"), the library retries only if the
condition is specified.

Rather than using this default behaviour, you may choose to disable the retries
Copy link

@tritone tritone Aug 16, 2022

Choose a reason for hiding this comment

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

Also include information about how users can enable retries on non-idempotent operations. (this is possible right?)

Copy link
Contributor Author

@bajajneha27 bajajneha27 Aug 18, 2022

Choose a reason for hiding this comment

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

We'll be adding it in a separate PR which covers retry configurations for non-idempotent operations overall. I didn't want to club that here because Sandeep has already worked on it.

on your own.

You can pass `retries` as `0` to disable retries for all operations regardless
of their idempotencies.

```ruby
require "google/cloud/storage"

storage = Google::Cloud::Storage.new retries: 0
```

You can also disable retries for a particular operation by passing `retries` as
`0` in the `options` field.

```ruby
require "google/cloud/storage"

storage = Google::Cloud::Storage.new
service = storage.service
service.get_bucket bucket_name, options: {retries: 0}
```

See the [Storage status and error
codes](https://cloud.google.com/storage/docs/json_api/v1/status-codes)
for a list of error conditions.
Expand Down
226 changes: 156 additions & 70 deletions google-cloud-storage/lib/google/cloud/storage/service.rb
View file
Open in desktop

Large diffs are not rendered by default.

20 changes: 10 additions & 10 deletions google-cloud-storage/samples/acceptance/hmac_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
mock.expect :list_project_hmac_keys,
hmac_keys_metadata_gapi,
[project], max_results: nil, page_token: nil, service_account_email: nil,
show_deleted_keys: nil, user_project: nil
show_deleted_keys: nil, user_project: nil, options: {}
storage.service.mocked_service = mock

Google::Cloud::Storage.stub :new, storage do
Expand All @@ -74,7 +74,7 @@

it "create_hmac_key" do
mock = Minitest::Mock.new
mock.expect :create_project_hmac_key, hmac_key_gapi, ["test", service_account_email], user_project: nil
mock.expect :create_project_hmac_key, hmac_key_gapi, ["test", service_account_email], user_project: nil, options: {}
storage.service.mocked_service = mock

Google::Cloud::Storage.stub :new, storage do
Expand All @@ -90,7 +90,7 @@

it "get_hmac_key" do
mock = Minitest::Mock.new
mock.expect :get_project_hmac_key, hmac_key_metadata_gapi, [project, access_id], user_project: nil
mock.expect :get_project_hmac_key, hmac_key_metadata_gapi, [project, access_id], user_project: nil, options: {}
storage.service.mocked_service = mock

Google::Cloud::Storage.stub :new, storage do
Expand All @@ -107,10 +107,10 @@
mock = Minitest::Mock.new
inactive_gapi = hmac_key_metadata_gapi.dup
inactive_gapi.state = "INACTIVE"
mock.expect :get_project_hmac_key, inactive_gapi, [project, access_id], user_project: nil
mock.expect :get_project_hmac_key, inactive_gapi, [project, access_id], user_project: nil, options: {}
# Expect any HmacKeyMetadata rather than `hmac_key_metadata_gapi` due to mock matching error.
mock.expect :update_project_hmac_key, hmac_key_metadata_gapi,
[project, access_id, Google::Apis::StorageV1::HmacKeyMetadata], user_project: nil
[project, access_id, Google::Apis::StorageV1::HmacKeyMetadata], user_project: nil, options: {}
storage.service.mocked_service = mock

Google::Cloud::Storage.stub :new, storage do
Expand All @@ -125,12 +125,12 @@

it "deactivate_hmac_key" do
mock = Minitest::Mock.new
mock.expect :get_project_hmac_key, hmac_key_metadata_gapi, [project, access_id], user_project: nil
mock.expect :get_project_hmac_key, hmac_key_metadata_gapi, [project, access_id], user_project: nil, options: {}
inactive_gapi = hmac_key_metadata_gapi.dup
inactive_gapi.state = "INACTIVE"
# Expect any HmacKeyMetadata rather than `inactive_gapi` due to mock matching error.
mock.expect :update_project_hmac_key, inactive_gapi,
[project, access_id, Google::Apis::StorageV1::HmacKeyMetadata], user_project: nil
[project, access_id, Google::Apis::StorageV1::HmacKeyMetadata], user_project: nil, options: {}
storage.service.mocked_service = mock

Google::Cloud::Storage.stub :new, storage do
Expand All @@ -145,9 +145,9 @@

it "delete_hmac_key" do
mock = Minitest::Mock.new
mock.expect :get_project_hmac_key, hmac_key_metadata_gapi, [project, access_id], user_project: nil
mock.expect :delete_project_hmac_key, hmac_key_metadata_gapi, [project, access_id], user_project: nil
mock.expect :get_project_hmac_key, hmac_key_metadata_gapi, [project, access_id], user_project: nil
mock.expect :get_project_hmac_key, hmac_key_metadata_gapi, [project, access_id], user_project: nil, options: {}
mock.expect :delete_project_hmac_key, hmac_key_metadata_gapi, [project, access_id], user_project: nil, options: {}
mock.expect :get_project_hmac_key, hmac_key_metadata_gapi, [project, access_id], user_project: nil, options: {}
storage.service.mocked_service = mock

Google::Cloud::Storage.stub :new, storage do
Expand Down
28 changes: 14 additions & 14 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 @@ -26,7 +26,7 @@
mock.expect :get_bucket, bucket_gapi, [bucket_name], **get_bucket_args
mock.expect :list_bucket_access_controls,
Google::Apis::StorageV1::BucketAccessControls.from_json(random_bucket_acl_hash(bucket_name).to_json),
[bucket_name], user_project: nil
[bucket_name], user_project: nil, options: {}

storage.service.mocked_service = mock

Expand All @@ -44,7 +44,7 @@
mock.expect :get_bucket, bucket_gapi, [bucket_name], **get_bucket_args(user_project: "test")
mock.expect :list_bucket_access_controls,
Google::Apis::StorageV1::BucketAccessControls.from_json(random_bucket_acl_hash(bucket_name).to_json),
[bucket_name], user_project: "test"
[bucket_name], user_project: "test", options: {}

storage.service.mocked_service = mock

Expand Down Expand Up @@ -74,10 +74,10 @@
mock.expect :get_bucket, bucket_gapi, [bucket_name], **get_bucket_args
mock.expect :list_bucket_access_controls,
Google::Apis::StorageV1::BucketAccessControls.from_json(random_bucket_acl_hash(bucket_name).to_json),
[bucket_name], user_project: nil
[bucket_name], user_project: nil, options: {}
mock.expect :insert_bucket_access_control,
Google::Apis::StorageV1::BucketAccessControl.from_json(writer_acl.to_json),
[bucket_name, Google::Apis::StorageV1::BucketAccessControl.new(entity: entity, role: "WRITER")], user_project: nil
[bucket_name, Google::Apis::StorageV1::BucketAccessControl.new(entity: entity, role: "WRITER")], user_project: nil, options: {}

storage.service.mocked_service = mock

Expand Down Expand Up @@ -113,7 +113,7 @@
mock.expect :get_bucket, bucket_gapi, [bucket_name], **get_bucket_args(user_project: "test")
mock.expect :insert_bucket_access_control,
Google::Apis::StorageV1::BucketAccessControl.from_json(acl_hash.to_json),
[bucket_name, Google::Apis::StorageV1::BucketAccessControl.new(entity: entity, role: "READER")], user_project: "test"
[bucket_name, Google::Apis::StorageV1::BucketAccessControl.new(entity: entity, role: "READER")], user_project: "test", options: {}

storage.service.mocked_service = mock

Expand Down Expand Up @@ -141,10 +141,10 @@
mock.expect :get_bucket, bucket_gapi, [bucket_name], **get_bucket_args(user_project: "test")
mock.expect :list_bucket_access_controls,
Google::Apis::StorageV1::BucketAccessControls.from_json(random_bucket_acl_hash(bucket_name).to_json),
[bucket_name], user_project: "test"
[bucket_name], user_project: "test", options: {}
mock.expect :insert_bucket_access_control,
Google::Apis::StorageV1::BucketAccessControl.from_json(acl_hash.to_json),
[bucket_name, Google::Apis::StorageV1::BucketAccessControl.new(entity: entity, role: "WRITER")], user_project: "test"
[bucket_name, Google::Apis::StorageV1::BucketAccessControl.new(entity: entity, role: "WRITER")], user_project: "test", options: {}

storage.service.mocked_service = mock

Expand Down Expand Up @@ -180,7 +180,7 @@
mock.expect :get_bucket, bucket_gapi, [bucket_name], **get_bucket_args(user_project: "test")
mock.expect :insert_bucket_access_control,
Google::Apis::StorageV1::BucketAccessControl.from_json(acl_hash.to_json),
[bucket_name, Google::Apis::StorageV1::BucketAccessControl.new(entity: entity, role: "OWNER")], user_project: "test"
[bucket_name, Google::Apis::StorageV1::BucketAccessControl.new(entity: entity, role: "OWNER")], user_project: "test", options: {}

storage.service.mocked_service = mock

Expand All @@ -198,9 +198,9 @@
mock.expect :get_bucket, bucket_gapi, [bucket_name], **get_bucket_args
mock.expect :list_bucket_access_controls,
Google::Apis::StorageV1::BucketAccessControls.from_json(random_bucket_acl_hash(bucket_name).to_json),
[bucket_name], user_project: nil
[bucket_name], user_project: nil, options: {}
mock.expect :delete_bucket_access_control, nil,
[bucket_name, existing_reader_entity], user_project: nil
[bucket_name, existing_reader_entity], user_project: nil, options: {}

storage.service.mocked_service = mock

Expand All @@ -226,9 +226,9 @@
mock.expect :get_bucket, bucket_gapi, [bucket_name], **get_bucket_args(user_project: "test")
mock.expect :list_bucket_access_controls,
Google::Apis::StorageV1::BucketAccessControls.from_json(random_bucket_acl_hash(bucket_name).to_json),
[bucket_name], user_project: "test"
[bucket_name], user_project: "test", options: {}
mock.expect :delete_bucket_access_control, nil,
[bucket_name, existing_reader_entity], user_project: "test"
[bucket_name, existing_reader_entity], user_project: "test", options: {}

storage.service.mocked_service = mock

Expand Down 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
22 changes: 11 additions & 11 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 Down Expand Up @@ -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
22 changes: 11 additions & 11 deletions 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 @@ -26,7 +26,7 @@
mock.expect :get_bucket, bucket_gapi, [bucket_name], **get_bucket_args
mock.expect :list_default_object_access_controls,
Google::Apis::StorageV1::ObjectAccessControls.from_json(random_default_acl_hash(bucket_name).to_json),
[bucket_name], user_project: nil
[bucket_name], user_project: nil, options: {}

storage.service.mocked_service = mock

Expand All @@ -43,7 +43,7 @@
mock.expect :get_bucket, bucket_gapi, [bucket_name], **get_bucket_args(user_project: "test")
mock.expect :list_default_object_access_controls,
Google::Apis::StorageV1::ObjectAccessControls.from_json(random_default_acl_hash(bucket_name).to_json),
[bucket_name], user_project: "test"
[bucket_name], user_project: "test", options: {}

storage.service.mocked_service = mock

Expand Down Expand Up @@ -72,10 +72,10 @@
mock.expect :get_bucket, bucket_gapi, [bucket_name], **get_bucket_args
mock.expect :list_default_object_access_controls,
Google::Apis::StorageV1::ObjectAccessControls.from_json(random_default_acl_hash(bucket_name).to_json),
[bucket_name], user_project: nil
[bucket_name], user_project: nil, options: {}
mock.expect :insert_default_object_access_control,
Google::Apis::StorageV1::BucketAccessControl.from_json(reader_acl.to_json),
[bucket_name, Google::Apis::StorageV1::BucketAccessControl.new(entity: reader_entity, role: "READER")], user_project: nil
[bucket_name, Google::Apis::StorageV1::BucketAccessControl.new(entity: reader_entity, role: "READER")], user_project: nil, options: {}

storage.service.mocked_service = mock

Expand Down Expand Up @@ -109,10 +109,10 @@
mock.expect :get_bucket, bucket_gapi, [bucket_name], **get_bucket_args(user_project: "test")
mock.expect :list_default_object_access_controls,
Google::Apis::StorageV1::ObjectAccessControls.from_json(random_default_acl_hash(bucket_name).to_json),
[bucket_name], user_project: "test"
[bucket_name], user_project: "test", options: {}
mock.expect :insert_default_object_access_control,
Google::Apis::StorageV1::BucketAccessControl.from_json(reader_acl.to_json),
[bucket_name, Google::Apis::StorageV1::BucketAccessControl.new(entity: reader_entity, role: "READER")], user_project: "test"
[bucket_name, Google::Apis::StorageV1::BucketAccessControl.new(entity: reader_entity, role: "READER")], user_project: "test", options: {}

storage.service.mocked_service = mock

Expand All @@ -136,9 +136,9 @@
mock.expect :get_bucket, bucket_gapi, [bucket_name], **get_bucket_args
mock.expect :list_default_object_access_controls,
Google::Apis::StorageV1::ObjectAccessControls.from_json(random_default_acl_hash(bucket_name).to_json),
[bucket_name], user_project: nil
[bucket_name], user_project: nil, options: {}
mock.expect :delete_default_object_access_control, nil,
[bucket_name, existing_reader_entity], user_project: nil
[bucket_name, existing_reader_entity], user_project: nil, options: {}

storage.service.mocked_service = mock

Expand All @@ -162,9 +162,9 @@
mock.expect :get_bucket, bucket_gapi, [bucket_name], **get_bucket_args(user_project: "test")
mock.expect :list_default_object_access_controls,
Google::Apis::StorageV1::ObjectAccessControls.from_json(random_default_acl_hash(bucket_name).to_json),
[bucket_name], user_project: "test"
[bucket_name], user_project: "test", options: {}
mock.expect :delete_default_object_access_control, nil,
[bucket_name, existing_reader_entity], user_project: "test"
[bucket_name, existing_reader_entity], user_project: "test", options: {}

storage.service.mocked_service = mock

Expand Down 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
Loading