Skip to content

Conversation

@shubhangi-google
Copy link
Owner

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:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea.
  • Follow the instructions in CONTRIBUTING. Most importantly, ensure the tests and linter pass by running bundle exec rake ci in the gem subdirectory.
  • Update code documentation if necessary.

closes: #<issue_number_goes_here>

@gemini-code-assist
Copy link

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 significantly expands the google-cloud-storage-control library's sample coverage by adding new examples and an acceptance test for Google Cloud Storage Anywhere Caches. The changes provide clear demonstrations of how to programmatically interact with Anywhere Cache resources, from creation and configuration to state management (pause/resume) and eventual disabling, ensuring developers have robust guidance for integrating this feature.

Highlights

  • New Anywhere Cache Samples: Introduced a comprehensive set of Ruby samples for managing Google Cloud Storage Anywhere Caches, covering create, list, get, update, pause, resume, and disable operations.
  • Anywhere Cache Acceptance Test: Added a new acceptance test to validate the full lifecycle of Anywhere Cache operations, ensuring proper functionality and integration.
  • Helper Method Enhancements: Updated the helper.rb file to include the google-cloud-storage-control client and introduced a count_anywhere_caches method for robust testing and cleanup.
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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description

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

  1. 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.

Copy link

@gemini-code-assist gemini-code-assist bot left a 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

Choose a reason for hiding this comment

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

critical

Debugging code (binding.pry) has been left in the codebase. This will pause execution when this code is run and should be removed before merging.

# @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

Choose a reason for hiding this comment

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

high

The bucket_name parameter has a default value of nil. If this method is called without a bucket name, it will construct an invalid parent string (projects/_/buckets/) on line 79, leading to an API error. The bucket_name should be a required parameter.

def count_anywhere_caches bucket_name

end

after :all do
delete_bucket_helper bucket_name until count_anywhere_caches(bucket_name).zero?

Choose a reason for hiding this comment

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

high

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

Comment on lines +43 to +57
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}"

Choose a reason for hiding this comment

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

medium

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

Comment on lines 27 to 28
parent = "projects/_/buckets/#{bucket_name}"
# Set project to "_" to signify global bucket

Choose a reason for hiding this comment

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

medium

The comment on line 28 explains the parent variable, but it is placed after the variable's definition. For better readability, comments should precede the code they describe.

  # Set project to "_" to signify global bucket
  parent = "projects/_/buckets/#{bucket_name}"

Comment on lines +45 to +59
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"

Choose a reason for hiding this comment

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

medium

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

@shubhangi-google
Copy link
Owner Author

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines +35 to +37
result.response.anywhere_caches.each do |item|
puts "AnywhereCache #{item.name} found in list"
end

Choose a reason for hiding this comment

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

high

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

Choose a reason for hiding this comment

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

medium

There is a trailing whitespace at the end of this line. It should be removed to adhere to common style guidelines.

def count_anywhere_caches bucket_name

Comment on lines +84 to +91
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

Choose a reason for hiding this comment

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

medium

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

Comment on lines +49 to +56
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

Choose a reason for hiding this comment

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

medium

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

Copy link
Collaborator

@thiyaguk09 thiyaguk09 left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants