feat(storage): Parameterized test setup for RCU Integration Testing - #14119
feat(storage): Parameterized test setup for RCU Integration Testing#14119nidhiii-27 wants to merge 4 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request introduces support for a new PREPROD backend and expands the integration testing framework to support combinations of LocationType and Colocation configurations, including dynamic bucket creation and Rapid Cache setup. The review feedback highlights critical compilation errors in CrossRunIntersection.java due to missing static imports for requireNonNull, a potential resource leak in BackendResources.java during cleanup if a bucket fails to stop, and a robustness improvement for URL parsing in HttpStorageRpc.java.
Sorry, something went wrong.
| } | ||
|
|
||
| public CrossRunIntersection withLocationType(LocationType locationType) { | ||
| requireNonNull(locationType, "locationType must be non null"); |
There was a problem hiding this comment.
The method requireNonNull is called here but it is not statically imported in this file, which will cause a compilation error. Please use Objects.requireNonNull instead.
| requireNonNull(locationType, "locationType must be non null"); | |
| Objects.requireNonNull(locationType, "locationType must be non null"); |
Sorry, something went wrong.
| } | ||
|
|
||
| public CrossRunIntersection withColocation(Colocation colocation) { | ||
| requireNonNull(colocation, "colocation must be non null"); |
There was a problem hiding this comment.
The method requireNonNull is called here but it is not statically imported in this file, which will cause a compilation error. Please use Objects.requireNonNull instead.
| requireNonNull(colocation, "colocation must be non null"); | |
| Objects.requireNonNull(colocation, "colocation must be non null"); |
Sorry, something went wrong.
| @Override | ||
| public void stop() { | ||
| protectedBucketNames.stop(); | ||
| dynamicBuckets.values().forEach(BucketInfoShim::stop); |
There was a problem hiding this comment.
If any of the dynamic buckets fail to stop (e.g., due to a network issue or if the bucket was already deleted), the forEach loop will terminate abruptly. This will prevent the remaining dynamic buckets from being stopped, leading to resource leaks. Wrapping the stop() call in a try-catch block ensures that all buckets are processed.
| dynamicBuckets.values().forEach(BucketInfoShim::stop); | |
| dynamicBuckets.values().forEach(shim -> { | |
| try { | |
| shim.stop(); | |
| } catch (Exception e) { | |
| // Suppress exception to ensure all dynamic buckets are stopped | |
| } | |
| }); |
- In test cleanup or resource deletion logic, catching a broad Exception is acceptable to ensure that the cleanup process is not halted by individual failures.
Sorry, something went wrong.
| java.net.URI uri = java.net.URI.create(host); | ||
| String path = uri.getPath(); | ||
| if (path != null && !path.isEmpty() && !"/".equals(path)) { | ||
| String rootUrl = host.substring(0, host.indexOf(path)); |
There was a problem hiding this comment.
Using host.indexOf(path) to find the start of the path is generally safe here, but using host.length() - path.length() is more robust, direct, and avoids searching the string, as the path is guaranteed to be at the end of the host URL.
| String rootUrl = host.substring(0, host.indexOf(path)); | |
| String rootUrl = host.substring(0, host.length() - path.length()); |
Sorry, something went wrong.
…se to ITAppendableUploadFakeTest Verify that successful responses on the BidiWriteObject stream (such as a successful chunk persist or reconnect state lookup response) correctly reset the client's consecutive redirect counter to 0. This ensures that the client is not blocked by the max consecutive redirect limit (3) when redirects are spread out. [Generated-by: AI]
Reformat modified and new test/runner files to comply with Spotify Java format checks in the CI pipelines. [Generated-by: AI]
Reviewers
1 more reviewerAt least 1 approving review is required to merge this pull request.
No description provided.