test: Add intgeration test for GoogleCloudStorageOuputStream and GCsFileSystemImpl#336
test: Add intgeration test for GoogleCloudStorageOuputStream and GCsFileSystemImpl#336dheerajsngh wants to merge 31 commits into
Conversation
# Conflicts: # client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsClientImpl.java # client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsClientImplTest.java
Summary of ChangesHello, 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 implements a new GoogleCloudStorageOutputStream to enable standard OutputStream-based writing to GCS. It extends the existing client and file system layers to support write channel creation and includes robust integration testing to ensure reliability across various write scenarios, such as parallel composite uploads and checksum validation. 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 the 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 counterproductive. 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. Footnotes
|
dheerajsngh
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #336 +/- ##
============================================
+ Coverage 97.43% 97.94% +0.51%
- Complexity 438 484 +46
============================================
Files 33 35 +2
Lines 1405 1512 +107
Branches 129 141 +12
============================================
+ Hits 1369 1481 +112
+ Misses 20 16 -4
+ Partials 16 15 -1
🚀 New features to boost your workflow:
|
![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 GoogleCloudStorageOutputStream to support writing objects to Google Cloud Storage, wrapping a WritableByteChannel with standard OutputStream semantics. It refactors GcsWriteOptions by moving upload-related configurations to GcsClientOptions and delegates GCS object creation from GcsFileSystem to GcsClient. Feedback on the changes highlights a compilation failure in GcsFileSystemImplIntegrationTest due to the refactored write options, a potential NoSuchElementException when creating blob info with a bucket-only ID, and a potential DirectoryNotEmptyException during test directory cleanup. Additionally, the reviewer suggests adding a defensive null check for the created channel and replacing magic numbers with reusable KB and MB constants in the integration tests.
| GcsFileSystemOptions options = GcsFileSystemOptions.builder() | ||
| .setGcsClientOptions(GcsClientOptions.builder().build()) | ||
| .build(); | ||
| GcsFileSystemImpl gcsFileSystem = new GcsFileSystemImpl(options); | ||
|
|
||
| GcsItemId itemId = GcsItemId.builder() | ||
| .setBucketName(bucketName) | ||
| .setObjectName(objectName) | ||
| .build(); | ||
|
|
||
| GcsWriteOptions writeOptions = GcsWriteOptions.builder() | ||
| .setUploadStrategy(GcsWriteOptions.UploadStrategy.PARALLEL_COMPOSITE_UPLOAD) | ||
| .build(); |
There was a problem hiding this comment.
The GcsWriteOptions class was refactored in this PR to remove upload configurations (such as UploadType and setUploadStrategy), moving them to GcsClientOptions. Consequently, calling setUploadStrategy on GcsWriteOptions.builder() will cause a compilation failure.
Please configure the upload type on GcsClientOptions instead.
| GcsFileSystemOptions options = GcsFileSystemOptions.builder() | |
| .setGcsClientOptions(GcsClientOptions.builder().build()) | |
| .build(); | |
| GcsFileSystemImpl gcsFileSystem = new GcsFileSystemImpl(options); | |
| GcsItemId itemId = GcsItemId.builder() | |
| .setBucketName(bucketName) | |
| .setObjectName(objectName) | |
| .build(); | |
| GcsWriteOptions writeOptions = GcsWriteOptions.builder() | |
| .setUploadStrategy(GcsWriteOptions.UploadStrategy.PARALLEL_COMPOSITE_UPLOAD) | |
| .build(); | |
| GcsFileSystemOptions options = GcsFileSystemOptions.builder() | |
| .setGcsClientOptions(GcsClientOptions.builder() | |
| .setUploadType(GcsClientOptions.UploadType.PARALLEL_COMPOSITE_UPLOAD) | |
| .build()) | |
| .build(); | |
| GcsFileSystemImpl gcsFileSystem = new GcsFileSystemImpl(options); | |
| GcsItemId itemId = GcsItemId.builder() | |
| .setBucketName(bucketName) | |
| .setObjectName(objectName) | |
| .build(); | |
| GcsWriteOptions writeOptions = GcsWriteOptions.builder().build(); |
| checkNotNull(itemId, "itemId should not be null"); | ||
|
|
||
| BlobInfo blobInfo = createBlobInfo(itemId); |
There was a problem hiding this comment.
To prevent a NoSuchElementException when calling createBlobInfo(itemId) with a bucket-only GcsItemId (where getObjectName() is empty), we should defensively validate that the itemId represents a GCS object before proceeding.
| checkNotNull(itemId, "itemId should not be null"); | |
| BlobInfo blobInfo = createBlobInfo(itemId); | |
| checkNotNull(itemId, "itemId should not be null"); | |
| checkArgument(itemId.isGcsObject(), "Expected GCS object to be provided. But got: " + itemId); | |
| BlobInfo blobInfo = createBlobInfo(itemId); |
| WritableByteChannel channel = gcsFileSystem.create(itemId, writeOptions); | ||
| return new GoogleCloudStorageOutputStream(channel); |
There was a problem hiding this comment.
Add a defensive null check for the returned WritableByteChannel to fail-fast during stream creation rather than throwing a NullPointerException later when write is called.
WritableByteChannel channel = gcsFileSystem.create(itemId, writeOptions);
checkNotNull(channel, "channel shouldn't be null");
return new GoogleCloudStorageOutputStream(channel);| assertThat(customFs.getFileInfo(uri).getItemInfo().getSize()).isEqualTo((long) TEST_CONTENT.length); | ||
| } | ||
| } finally { | ||
| Files.deleteIfExists(tempDir); |
There was a problem hiding this comment.
Using Files.deleteIfExists(tempDir) on a non-empty directory will throw a DirectoryNotEmptyException and fail to clean up the temporary directory, leading to leaked files on the build agent. Since journaling writes files into this directory, we should delete it recursively.
if (Files.exists(tempDir)) {
try (java.util.stream.Stream<Path> walk = Files.walk(tempDir)) {
walk.sorted(java.util.Comparator.reverseOrder())
.map(Path::toFile)
.forEach(java.io.File::delete);
}
}| private static final String SUFFIX_TXT = ".txt"; | ||
| private static final String SUFFIX_CSV = ".csv"; | ||
| private static final String SUFFIX_BIN = ".bin"; | ||
| private static final String SUFFIX_PARQUET = ".parquet"; |
There was a problem hiding this comment.
Define reusable constants for KB and MB at the class level to avoid using raw arithmetic expressions and magic numbers for sizes and limits, in accordance with the repository's general rules.
private static final String SUFFIX_TXT = ".txt";
private static final String SUFFIX_CSV = ".csv";
private static final String SUFFIX_BIN = ".bin";
private static final String SUFFIX_PARQUET = ".parquet";
private static final int KB = 1024;
private static final int MB = 1024 * 1024;References
- Avoid using hardcoded magic numbers or raw arithmetic expressions for sizes and limits in tests. Instead, define and use constants (such as KB = 1024) to make the code more readable and maintainable.
| GcsClientOptions clientOptions = GcsClientOptions.builder() | ||
| .setUploadChunkSize(256 * 1024) | ||
| .build(); | ||
| GcsWriteOptions writeOptions = GcsWriteOptions.builder().build(); | ||
|
|
||
| int totalSize = 1024 * 1024; // 1 MB total size | ||
| byte[] chunk = new byte[1024]; // 1 KB chunks written locally |
There was a problem hiding this comment.
Use the defined KB and MB constants instead of raw arithmetic expressions and magic numbers.
| GcsClientOptions clientOptions = GcsClientOptions.builder() | |
| .setUploadChunkSize(256 * 1024) | |
| .build(); | |
| GcsWriteOptions writeOptions = GcsWriteOptions.builder().build(); | |
| int totalSize = 1024 * 1024; // 1 MB total size | |
| byte[] chunk = new byte[1024]; // 1 KB chunks written locally | |
| GcsClientOptions clientOptions = GcsClientOptions.builder() | |
| .setUploadChunkSize(256 * KB) | |
| .build(); | |
| GcsWriteOptions writeOptions = GcsWriteOptions.builder().build(); | |
| int totalSize = 1 * MB; // 1 MB total size | |
| byte[] chunk = new byte[KB]; // 1 KB chunks written locally |
References
- Avoid using hardcoded magic numbers or raw arithmetic expressions for sizes and limits in tests. Instead, define and use constants (such as KB = 1024) to make the code more readable and maintainable.
| GcsClientOptions clientOptions = GcsClientOptions.builder() | ||
| .setUploadType(GcsClientOptions.UploadType.PARALLEL_COMPOSITE_UPLOAD) | ||
| .setPcuBufferCount(2) | ||
| .setPcuBufferCapacity(16 * 1024 * 1024) // 16MB |
There was a problem hiding this comment.
Use the defined MB constant instead of raw arithmetic expressions.
| .setPcuBufferCapacity(16 * 1024 * 1024) // 16MB | |
| .setPcuBufferCapacity(16 * MB) |
References
- Avoid using hardcoded magic numbers or raw arithmetic expressions for sizes and limits in tests. Instead, define and use constants (such as KB = 1024) to make the code more readable and maintainable.
1 participant
Type of Change
feat: A new featurefix: A bug fixdocs: Documentation only changesstyle: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)refactor: A code change that neither fixes a bug nor adds a featureperf: A code change that improves performancetest: Adding missing tests or correcting existing testschore: Changes to the build process or auxiliary tools and libraries such as documentation generationDescription
What?
GoogleCloudStorageOutputStreamIntegrationTest) to verify the functionality ofGoogleCloudStorageOutputStream.TestInputStreamInputFile,TestOutputStreamOutputFile, andIntegrationTestHelper) to help validate data writes, read-backs, and Parquet formatting.GcsFileSystemImplIntegrationTestfor testing file creation with different configurationsWhy?
Verify write behavior for different scenarios.
Checklist
test(core): add integration tests for GCS writes)Generated/Assisted by Agent? Yes