Repository navigation
test(storage): Parameterized test setup for RCU integration testing - #14530
nidhiii-27 wants to merge 5 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for a new PREPROD backend and adds a LocationType enum to support regional standard, regional rapid, and zonal rapid bucket topologies in integration tests. It also updates HttpStorageRpc to parse custom hosts with subpaths into distinct root URLs and service paths. The review feedback highlights two key improvements: first, using uri.getRawPath() instead of uri.getPath() in HttpStorageRpc to avoid potential StringIndexOutOfBoundsException issues with percent-encoded characters; second, overriding the target region and zone for ZONAL_RAPID buckets (similar to REGIONAL_RAPID) to prevent test failures in zones that do not support the rapid cache storage class.
| } | ||
| storageBuilder.setRootUrl(rootUrl); | ||
| storageBuilder.setServicePath(servicePath); | ||
| } else { |
There was a problem hiding this comment.
This change is in production code, so I think it might change behavior for existing users who set a host with a path, e.g. https://proxy.example.com/gcs/. Today the whole host is the root URL, so requests go to https://proxy.example.com/gcs/storage/v1/b/.... With this change the path replaces storage/v1/, so the same config sends requests to https://proxy.example.com/gcs/b/....
There was a problem hiding this comment.
Good catch! Updated the host parsing logic in HttpStorageRpc so that servicePath is only overridden when the URI path explicitly contains a /storage/v1... segment (such as /storage/v1_preprod/), preserving the existing rootUrl behavior for reverse-proxy paths like https://proxy.example.com/gcs/.
Co-authored by AI Agent
There was a problem hiding this comment.
The check is indexOf("/storage/v1"), a substring match, so hosts like https://proxy/storage/v1-mirror/ or /storage/v1beta/ still change from storage/v1/b/... to b/...
There was a problem hiding this comment.
Restricted the service path split in HttpStorageRpc to match only /storage/v1_preprod (or /storage/v1_preprod/), and added unit tests in HttpStorageRpcUriTest covering /storage/v1-mirror/ and /storage/v1beta/.
Co-authored by AI Agent
There was a problem hiding this comment.
Looks safe to me but since HttpStorageRpc ships to customers, but under the test: commit type release-please leaves it out of the changelog, to be doubly sure can we validate that this has no impact on users.
There was a problem hiding this comment.
The PR description still says "custom /storage/v1* service paths". It should say only /storage/v1_preprod can you update it accordingly
There was a problem hiding this comment.
Validated that this change has zero impact on users:
- Code path:
ServiceOptionsandHttpStorageOptions.Builder.build()always resolveoptions.getHost()to a non-null string (defaulting to"https://storage.googleapis.com/"or"https://storage.<universeDomain>/"), andHttpStorageRpconly splitsrootUrl/servicePathwhennormalizedPath.endsWith("/storage/v1_preprod"). Every other host (default prod, custom universe domain, local TestBench/emulator, IPv6, reverse-proxy paths, or even malformed URI strings) executesstorageBuilder.setRootUrl(host), which is identical tomain. - Baseline equivalence test: Added
nonPreprodHosts_matchMainBaselineExactlyinHttpStorageRpcUriTestcomparingHttpStorageRpc'sStorageclient againstmain'snew Storage.Builder(...).setRootUrl(options.getHost()).build()(getRootUrl(),getServicePath(),getBaseUrl(), andbuildHttpRequestUrl()) across all user-facingStorageOptionsconfigurations.
Co-authored by AI Agent
There was a problem hiding this comment.
Done. Updated the PR description to specify /storage/v1_preprod only.
Co-authored by AI Agent
nidhiii-27
changed the title
fb05982 to
ad8cda7
Compare
Dhriti07
left a comment
Dhriti07
left a comment
There was a problem hiding this comment.
LGTM, subject to double validation that HttpStorageRcp changes have no user impact and are test only
- Add LocationType enum and integrate with CrossRun / SingleBackend annotations and CrossRunIntersection - Update StorageITRunner to support location-based parameterized permutations - Add PREPROD backend resources and dynamic bucket lifecycle management in BackendResources - Add RapidCache lifecycle support and cleanup in BucketInfoShim - Update HttpStorageRpc to properly handle custom subpaths in endpoint URIs - Add unit tests for HttpStorageRpc endpoint URI handling [Generated-by: AI]
…cation 1. Use uri.getRawPath() in HttpStorageRpc to handle raw percent-encoded URI paths. 2. Include LocationType.ZONAL_RAPID when configuring target region/zone for RCU buckets. [Generated-by: AI]
…adoc [Generated-by: AI]
- Restrict HttpStorageRpc custom service path split to /storage/v1_preprod and add unit tests for /storage/v1-mirror/ and /storage/v1beta/ - Simplify BucketInfoShim and extract RapidCacheHelper with selective capacity/availability skip handling and InterruptedException restoration - Memoize per-location setup failures/skips in DynamicBucketLifecycle and resolve default ObjectsFixture dynamically per LocationType [Generated-by: AI]
…reprod hosts [Generated-by: AI]
96f6bbe to
695027e
Compare
Adds location-based parameterization and backend setup to
StorageITRunnerfor Rapid Cache Ultra (RCU) integration testing:LocationTypeParameterization: AddsLocationType(US_MULTI_REGIONAL,REGIONAL_RAPID,ZONAL_RAPID) to@CrossRun,@CrossRun.Exclude,@CrossRun.Ignore,@SingleBackend, andStorageITRunner, combining class- and method-level ignore/exclude rules.DynamicBucketLifecycleinBackendResourcesandRapidCacheprovisioning/teardown inBucketInfoShim, along withPREPRODbackend support.HttpStorageRpc): Supports the/storage/v1_preprodservice path while preserving existingrootUrlbehavior for all other hosts and proxy paths (HttpStorageRpcUriTest).