Anywhere cache samples v2 #39
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @shubhangi-google, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request focuses on expanding the sample code for Google Cloud Storage's Anywhere Cache feature. It provides new Ruby examples demonstrating various lifecycle management operations for these caches, ensuring developers have clear guidance on how to interact with this service programmatically. The changes also include an acceptance test framework to support future testing of these long-running operations.
Highlights
- New Anywhere Cache Samples: This PR introduces a comprehensive set of Ruby samples for managing Google Cloud Storage Anywhere Caches, including operations for creation, listing, retrieval, pausing, resuming, disabling, and updating caches.
- Anywhere Cache Acceptance Test: An acceptance test file has been added for Anywhere Cache operations, though most tests are currently commented out due to the long-running nature of cache operations.
- README Update: The
README.mdfile has been updated with a note explaining why tests for Anywhere Cache samples are not included in the automated test suite.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request adds new samples for the Anywhere Cache feature in Storage Control. The changes are a good starting point, but there are several areas for improvement across the new sample files, particularly regarding long-running operation handling, pagination, error handling, and code style. I've left specific comments with suggestions to address these points.
| before :all do | ||
| @bucket= storage_client.bucket bucket_name | ||
| @bucket = create_bucket_helper bucket_name | ||
| end |
There was a problem hiding this comment.
This before :all block has a few issues:
- It attempts to use
storage_clientandbucket_name, which are defined withletand are not available in abefore :allcontext. This will cause the test setup to fail. You should use abefore(orbefore :each) block instead. - Line 33 is redundant as
@bucketis immediately reassigned on the next line. - Line 34 has an extra leading space, which affects readability.
before do
@bucket = create_bucket_helper bucket_name
end| begin | ||
| result = storage_control_client.list_anywhere_caches request | ||
| result.response.anywhere_caches.each do |item| | ||
| puts item.name | ||
| end | ||
| rescue StandardError => e | ||
| puts "Error listing AnywhereCaches: #{e.message}" | ||
| end |
There was a problem hiding this comment.
The current implementation result.response.anywhere_caches.each will only process the first page of results from the list operation. To correctly iterate through all pages of results, you should iterate directly over the PagedEnumerable object returned by the client.
I also suggest improving the output to be more user-friendly and rescuing a more specific error type than StandardError.
begin
puts "Listing Anywhere Caches for bucket \"#{bucket_name}\"..."
result = storage_control_client.list_anywhere_caches request
result.each do |cache|
puts cache.name
end
rescue Google::Cloud::Error => e
puts "Error listing AnywhereCaches: #{e.message}"
end| begin | ||
| operation = storage_control_client.create_anywhere_cache request | ||
|
|
||
| puts "********************AnywhereCache operation created - #{operation.name}" | ||
| while !operation.done? | ||
| sleep 3600 # Wait for 1 hour before checking again | ||
| operation.refresh! | ||
| end | ||
| puts "********************AnywhereCache create operation completed - #{operation.name}" | ||
|
|
||
| rescue StandardError => e | ||
| puts "Error creating AnywhereCache: #{e.message}" | ||
| end |
There was a problem hiding this comment.
The current implementation for handling the long-running operation is not ideal. Sleeping for a fixed hour is inefficient and provides a poor user experience. The client library provides a much better way to handle this using operation.wait_until_done!.
This method polls the operation status with exponential backoff and also simplifies error handling. Additionally, rescuing StandardError is too broad; it's better to rescue more specific exceptions like Google::Cloud::Error.
The output messages could also be more descriptive.
begin
operation = storage_control_client.create_anywhere_cache request
puts "Waiting for Anywhere Cache creation operation to complete... (name: #{operation.name})"
operation.wait_until_done!
if operation.error?
puts "Error creating AnywhereCache: #{operation.error.message}"
else
puts "AnywhereCache created: #{operation.response.name}"
end
rescue Google::Cloud::Error => e
puts "Error creating AnywhereCache: #{e.message}"
end| # Create a client object. The client can be reused for multiple calls. | ||
| storage_control_client = Google::Cloud::Storage::Control.storage_control | ||
| parent = "projects/_/buckets/#{bucket_name}" | ||
| name = "#{parent}/anywhereCaches/#{anywhere_cache_id}" |
There was a problem hiding this comment.
There's an extra space in this assignment, which should be removed for code consistency.
name = "#{parent}/anywhereCaches/#{anywhere_cache_id}"| # Create a client object. The client can be reused for multiple calls. | ||
| storage_control_client = Google::Cloud::Storage::Control.storage_control | ||
| parent = "projects/_/buckets/#{bucket_name}" | ||
| name = "#{parent}/anywhereCaches/#{anywhere_cache_id}" |
There was a problem hiding this comment.
There's an extra space in this assignment, which should be removed for code consistency.
name = "#{parent}/anywhereCaches/#{anywhere_cache_id}"| # Create a client object. The client can be reused for multiple calls. | ||
| storage_control_client = Google::Cloud::Storage::Control.storage_control | ||
| parent = "projects/_/buckets/#{bucket_name}" | ||
| name = "#{parent}/anywhereCaches/#{anywhere_cache_id}" |
There was a problem hiding this comment.
There's an extra space in this assignment, which should be removed for code consistency.
name = "#{parent}/anywhereCaches/#{anywhere_cache_id}"| begin | ||
| result = storage_control_client.pause_anywhere_cache request | ||
| puts "AnywhereCache #{result.name} #{result.state}" | ||
| rescue StandardError => e | ||
| puts "Error pausing AnywhereCache: #{e.message}" | ||
| end |
There was a problem hiding this comment.
This block can be improved by:
- Rescuing a more specific
Google::Cloud::Errorinstead ofStandardError. - Providing a more user-friendly output message.
begin
result = storage_control_client.pause_anywhere_cache request
puts "AnywhereCache #{result.name} is now #{result.state.to_s.downcase}."
rescue Google::Cloud::Error => e
puts "Error pausing AnywhereCache: #{e.message}"
end| begin | ||
| result = storage_control_client.resume_anywhere_cache request | ||
| puts "AnywhereCache #{result.name} #{result.state}" | ||
| rescue StandardError => e | ||
| puts "Error resuming AnywhereCache: #{e.message}" | ||
| end |
There was a problem hiding this comment.
This block can be improved by:
- Rescuing a more specific
Google::Cloud::Errorinstead ofStandardError. - Providing a more user-friendly output message. The current state is
ACTIVE, but the commented-out test expectsrunning. A clearer message would be better for users.
begin
result = storage_control_client.resume_anywhere_cache request
puts "AnywhereCache #{result.name} has been resumed and is now #{result.state.to_s.downcase}."
rescue Google::Cloud::Error => e
puts "Error resuming AnywhereCache: #{e.message}"
end| # bucket_name = "your-unique-bucket-name" | ||
| # A value that, along with the bucket's name, uniquely identifies the cache | ||
| # anywhere_cache_id = "us-east1-b" | ||
| require "google/cloud/storage/control/v2" |
There was a problem hiding this comment.
It's best practice to place all require statements at the top of the file. This require should be moved out of the method definition to the top of the file with the other require statements.
| begin | ||
| result = storage_control_client.update_anywhere_cache request | ||
| puts "AnywhereCache updated - #{result.name}" | ||
| rescue StandardError => e | ||
| puts "Error updating AnywhereCache: #{e.message}" | ||
| end |
There was a problem hiding this comment.
This block can be improved by:
- Rescuing a more specific
Google::Cloud::Errorinstead ofStandardError. - Providing a more informative output message that confirms what was updated.
begin
result = storage_control_client.update_anywhere_cache request
puts "Updated AnywhereCache #{result.name}, new TTL is #{result.ttl.seconds} seconds."
rescue Google::Cloud::Error => e
puts "Error updating AnywhereCache: #{e.message}"
end
|
/Gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new set of samples for the Anywhere Cache feature in Google Cloud Storage Control, along with corresponding acceptance tests. While the additions are valuable, the current implementation has several significant issues that need to be addressed. The acceptance test contains flawed cleanup logic and an extremely long, hardcoded sleep duration, rendering it broken and slow. Multiple samples that interact with long-running operations (LROs) either handle them with excessively long sleep intervals (e.g., 30 minutes in the create sample) or incorrectly treat them as synchronous calls (disable, pause, resume). Additionally, the update sample has a race condition. My review provides specific suggestions to fix these correctness and performance problems, primarily by properly handling LROs using the client library's built-in waiting mechanisms.
| after do | ||
| delete_bucket_helper bucket_name until count_anywhere_caches bucket_name | ||
| end |
There was a problem hiding this comment.
The cleanup logic in this after block is flawed. The until count_anywhere_caches bucket_name condition will always be true since count_anywhere_caches returns an integer (which is truthy in Ruby), so the delete_bucket_helper will only run once. More importantly, delete_bucket_helper will likely fail because the bucket still contains an Anywhere Cache instance (even a disabled one), and count_anywhere_caches introduces a 15-minute delay. The test is currently broken and will be very slow. The cleanup logic needs to be rewritten to correctly wait for resources to be cleaned up before attempting to delete the bucket.
| end | ||
|
|
||
| def count_anywhere_caches bucket_name | ||
| sleep 900 |
There was a problem hiding this comment.
A hardcoded sleep of 900 seconds (15 minutes) is too long for an acceptance test and will significantly slow down the CI process. Please replace this with a polling mechanism that checks the status more frequently (e.g., every few seconds) with a reasonable timeout.
| until result.state == "running" | ||
| sleep 1800 # Wait for 1/2 hour before checking again | ||
| result = storage_control_client.get_anywhere_cache get_request | ||
| puts "AnywhereCache status check retried" | ||
| end |
There was a problem hiding this comment.
The polling loop to check for cache creation status includes sleep 1800, which is 30 minutes. This will cause the sample script to hang for a very long time, providing a poor user experience. Instead of a manual polling loop with such a long sleep, please use the wait_until_done! method on the Gapic::Operation object (operation). This will handle polling efficiently. If you must poll manually, use a much shorter sleep interval (e.g., 10-30 seconds) and provide feedback to the user that the script is waiting.
puts "AnywhereCache operation created - #{operation.name}, waiting for it to complete."
operation.wait_until_done!
raise operation.error if operation.error?
result = operation.result| result = storage_control_client.disable_anywhere_cache request | ||
| puts "AnywhereCache #{result.name} #{result.state} " |
There was a problem hiding this comment.
The disable_anywhere_cache method returns a Gapic::Operation for a long-running operation, not the AnywhereCache resource directly. The current code treats it as a synchronous call, which is incorrect. You should handle the LRO, for example by calling wait_until_done! on the returned operation object to wait for completion, and then get the result. Also, there is a trailing space in the puts statement.
operation = storage_control_client.disable_anywhere_cache request
puts "Waiting for disable operation to complete..."
operation.wait_until_done!
raise operation.error if operation.error?
result = operation.result
puts "AnywhereCache #{result.name} #{result.state}"| result = storage_control_client.pause_anywhere_cache request | ||
| puts "AnywhereCache #{result.name} #{result.state}" |
There was a problem hiding this comment.
The pause_anywhere_cache method returns a Gapic::Operation for a long-running operation. The code incorrectly treats it as a synchronous call that returns the final resource. You need to handle the LRO, for instance by calling wait_until_done! on the operation to wait for it to finish.
operation = storage_control_client.pause_anywhere_cache request
puts "Waiting for pause operation to complete..."
operation.wait_until_done!
raise operation.error if operation.error?
result = operation.result
puts "AnywhereCache #{result.name} #{result.state}"| result = storage_control_client.resume_anywhere_cache request | ||
| puts "AnywhereCache #{result.name} #{result.state}" |
There was a problem hiding this comment.
The resume_anywhere_cache method returns a Gapic::Operation for a long-running operation. The code incorrectly treats it as a synchronous call. Please handle the LRO, for example by calling wait_until_done! on the returned operation object.
operation = storage_control_client.resume_anywhere_cache request
puts "Waiting for resume operation to complete..."
operation.wait_until_done!
raise operation.error if operation.error?
result = operation.result
puts "AnywhereCache #{result.name} #{result.state}"| puts "AnywhereCache operation created - #{operation.name}" | ||
| get_request = Google::Cloud::Storage::Control::V2::GetAnywhereCacheRequest.new( | ||
| name: name | ||
| ) | ||
| result = storage_control_client.get_anywhere_cache get_request | ||
| puts "AnywhereCache #{result.name} updated" |
There was a problem hiding this comment.
The code initiates an update operation but doesn't wait for it to complete. It immediately calls get_anywhere_cache, which creates a race condition. The get call might return the state of the cache before the update is finished. You should wait for the operation to complete using operation.wait_until_done! before fetching the resource to confirm the update.
puts "AnywhereCache operation created - #{operation.name}, waiting for it to complete."
operation.wait_until_done!
raise operation.error if operation.error?
result = operation.result
puts "AnywhereCache #{result.name} updated"
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
bundle exec rake ciin the gem subdirectory.closes: #<issue_number_goes_here>