-
Notifications
You must be signed in to change notification settings - Fork 0
Anywhere cache samples v2 new #42
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @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 significantly expands the Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
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.
Code Review
This pull request introduces a comprehensive set of samples for the Anywhere Cache feature in Google Cloud Storage Control API, along with corresponding acceptance tests. My review focuses on ensuring the correctness of the implementation, adherence to client library best practices, and cleaning up development artifacts. I've identified a critical debugging statement that needs removal, some logic issues in the test helpers and tests that could lead to failures or inefficiencies, and opportunities to refactor the long-running operation handling in the samples to be more idiomatic and robust.
|
|
||
| # Customize the options with defaults | ||
| metadata = @config.rpcs.get_operation.metadata.to_h | ||
| binding.pry |
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.
| # @param bucket_name [String] The name of the Google Cloud Storage bucket. | ||
| # @return [Integer] The final count of Anywhere Caches, which will be 0 | ||
| # the method completes successfully after all caches are deleted. | ||
| def count_anywhere_caches bucket_name = nil |
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.
| end | ||
|
|
||
| after :all do | ||
| delete_bucket_helper bucket_name until count_anywhere_caches(bucket_name).zero? |
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.
The cleanup logic in the after :all hook is incorrect. The until loop will repeatedly call delete_bucket_helper even if it fails due to existing caches, while count_anywhere_caches introduces long waits between attempts. This is inefficient and may lead to flaky tests. You should first wait for all caches to be deleted, and then delete the bucket once.
count_anywhere_caches(bucket_name)
delete_bucket_helper bucket_name| operation = storage_control_client.create_anywhere_cache request | ||
| 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 | ||
| min_delay = 180 # 3 minutes | ||
| max_delay = 900 # 15 minutes | ||
| while result.state != "running" | ||
| puts "Cache not running yet, current state is #{result.state}. Retrying in #{min_delay} seconds." | ||
| sleep min_delay | ||
| min_delay = [min_delay * 2, max_delay].min # Exponential backoff with a max delay | ||
| result = storage_control_client.get_anywhere_cache get_request | ||
| end | ||
| puts "AnywhereCache created - #{result.name}" |
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 sample uses a manual polling loop with get_anywhere_cache to wait for the create_anywhere_cache operation to complete. The client library provides a more robust and idiomatic way to handle long-running operations (LROs) using operation.wait_until_done!. This simplifies the code, makes it less error-prone, and uses the library's built-in exponential backoff mechanism.
operation = storage_control_client.create_anywhere_cache request
puts "AnywhereCache operation created - #{operation.name}"
operation.wait_until_done!
if operation.error?
puts "Error creating AnywhereCache: #{operation.error.message}"
else
result = operation.response
puts "AnywhereCache created - #{result.name}"
end| parent = "projects/_/buckets/#{bucket_name}" | ||
| # Set project to "_" to signify global bucket |
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.
| operation = storage_control_client.update_anywhere_cache request | ||
| 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 | ||
| min_delay = 120 # 2 minutes | ||
| max_delay = 600 # 10 minutes | ||
| while result.state != "running" | ||
| puts "Cache not running yet, current state is #{result.state}. Retrying in #{min_delay} seconds." | ||
| sleep min_delay | ||
| min_delay = [min_delay * 2, max_delay].min # Exponential backoff with a max delay | ||
| result = storage_control_client.get_anywhere_cache get_request | ||
| end | ||
| puts "AnywhereCache #{result.name} updated" |
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 sample uses a manual polling loop with get_anywhere_cache to wait for the update_anywhere_cache operation to complete. The client library provides a more robust and idiomatic way to handle long-running operations (LROs) using operation.wait_until_done!. This simplifies the code and avoids potential race conditions.
operation = storage_control_client.update_anywhere_cache request
puts "AnywhereCache operation created - #{operation.name}"
operation.wait_until_done!
if operation.error?
puts "Error updating AnywhereCache: #{operation.error.message}"
else
result = operation.response
puts "AnywhereCache #{result.name} updated"
end|
/gemini review |
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.
Code Review
This pull request introduces a comprehensive set of samples and corresponding acceptance tests for the Anywhere Cache feature in the Storage Control API. The changes are well-structured, covering the full lifecycle of a cache. My review includes a critical fix for a pagination issue in the list sample, along with several suggestions to improve code clarity and maintainability by refactoring polling logic, removing magic numbers, and cleaning up redundant code.
| result.response.anywhere_caches.each do |item| | ||
| puts "AnywhereCache #{item.name} found in list" | ||
| end |
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.
The current implementation result.response.anywhere_caches.each only iterates over the first page of results. To correctly handle pagination and process all caches, you should iterate directly over the Gapic::PagedEnumerable object that list_anywhere_caches returns.
result.each do |item|
puts "AnywhereCache #{item.name} found in list"
end| # @param bucket_name [String] The name of the Google Cloud Storage bucket. | ||
| # @return [Integer] The final count of Anywhere Caches, which will be 0 | ||
| # the method completes successfully after all caches are deleted. | ||
| def count_anywhere_caches bucket_name |
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.
| min_delay = 180 # 3 minutes | ||
| max_delay = 900 # 15 minutes | ||
| while result.response.anywhere_caches.count != 0 | ||
| puts "Cache not deleted yet, Retrying in #{min_delay} seconds." | ||
| sleep min_delay | ||
| min_delay = [min_delay * 2, max_delay].min # Exponential backoff with a max delay | ||
| result = storage_control_client.list_anywhere_caches request | ||
| end |
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.
The polling logic can be made clearer. The variable min_delay is being modified within the loop, which can be confusing as its name implies a fixed minimum. It's better to use a separate variable for the current delay to improve readability.
Additionally, the delay values 180 and 900 are magic numbers. It's a good practice to define them as constants with descriptive names (e.g., INITIAL_DELAY_SECONDS, MAX_DELAY_SECONDS).
initial_delay = 180 # 3 minutes
max_delay = 900 # 15 minutes
current_delay = initial_delay
while result.response.anywhere_caches.count != 0
puts "Cache not deleted yet, Retrying in #{current_delay} seconds."
sleep current_delay
current_delay = [current_delay * 2, max_delay].min # Exponential backoff with a max delay
result = storage_control_client.list_anywhere_caches request
end| min_delay = 180 # 3 minutes | ||
| max_delay = 900 # 15 minutes | ||
| while result.state != "running" | ||
| puts "Cache not running yet, current state is #{result.state}. Retrying in #{min_delay} seconds." | ||
| sleep min_delay | ||
| min_delay = [min_delay * 2, max_delay].min # Exponential backoff with a max delay | ||
| result = storage_control_client.get_anywhere_cache get_request | ||
| end |
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 polling logic can be improved for clarity. The variable min_delay is being modified within the loop, which can be confusing as its name implies a fixed minimum. Using a separate variable for the current delay makes the code easier to understand.
Also, the delay values 180 and 900 are magic numbers. They should be defined as constants with descriptive names to improve maintainability.
initial_delay = 180 # 3 minutes
max_delay = 900 # 15 minutes
current_delay = initial_delay
while result.state != "running"
puts "Cache not running yet, current state is #{result.state}. Retrying in #{current_delay} seconds."
sleep current_delay
current_delay = [current_delay * 2, max_delay].min # Exponential backoff with a max delay
result = storage_control_client.get_anywhere_cache get_request
end
google-cloud-storage-control/samples/storage_control_disable_anywhere_cache.rb
Outdated
Show resolved
Hide resolved
google-cloud-storage-control/samples/storage_control_update_anywhere_cache.rb
Outdated
Show resolved
Hide resolved
google-cloud-storage-control/samples/storage_control_update_anywhere_cache.rb
Show resolved
Hide resolved
google-cloud-storage-control/samples/storage_control_update_anywhere_cache.rb
Show resolved
Hide resolved
google-cloud-storage-control/samples/storage_control_update_anywhere_cache.rb
Outdated
Show resolved
Hide resolved
thiyaguk09
left a comment
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.
LGTM. @shubhangi-google Try to reduce the number of commits, as having 36 commits may create a negative impression. After raising the PR, avoid pushing commits continuously—only push if there is a major change or something important was missed, as frequent small updates can reflect poorly on your PR.
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>