feat: Extract metadata from GCS read channel#333
Conversation
…ed GcsFileInfo.fromItemInfo method
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 introduces a mechanism to extract file metadata (size and generation) from the GCS SDK ReadChannel using reflection. By capturing this information after the initial read, the system can perform optimizations like Parquet footer prefetching without requiring upfront metadata, improving performance in scenarios where such details are not provided by the user. 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
|
![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 dynamic metadata extraction (size and generation) from GCS SDK ReadChannel instances using reflection via a new GcsReadChannelMetadataExtractor utility. This allows GcsReadChannel to update its metadata after reading when itemInfo is not initially provided, which is then utilized by GcsFooterOptimizer and other components. The code reviewer identified several critical issues: a potential crash from unhandled CancellationException when resolving futures, a performance bottleneck where failed metadata extractions trigger reflection on every subsequent chunk read, and concurrency issues (data races) due to non-volatile fields (itemInfo and itemId) during concurrent reads. Additionally, the reviewer recommended excluding Collection types from fallback size extraction to prevent incorrect size values.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #333 +/- ##
============================================
+ Coverage 97.44% 97.62% +0.18%
- Complexity 439 482 +43
============================================
Files 33 35 +2
Lines 1407 1516 +109
Branches 129 152 +23
============================================
+ Hits 1371 1480 +109
+ Misses 20 19 -1
- Partials 16 17 +1
🚀 New features to boost your workflow:
|
| private int readNextChunk(ByteBuffer dst) throws IOException { | ||
| ReadChannel sdkChannel = strategy.getReadChannel(gcsReadChannelPosition, dst.remaining()); | ||
| int bytesRead = sdkChannel.read(dst); | ||
| if (this.itemInfo == null && !metadataExtractionAttempted) { |
| int numOfBytesRead = 0; | ||
| while (dataBuffer.hasRemaining()) { | ||
| int bytesRead = channel.read(dataBuffer); | ||
| if (GcsReadChannel.this.itemInfo == null && !metadataExtractionAttempted) { |
There was a problem hiding this comment.
Drop "GcsReadChannel.this."
|
|
||
| private synchronized boolean extractMetadataAfterRead(ReadStrategy strategy) { | ||
| metadataExtractionAttempted = true; | ||
| GcsReadChannelMetadataExtractor.ExtractedMetadata metadata = |
There was a problem hiding this comment.
Any reason for keeping this type qualified ?
| return true; | ||
| } | ||
|
|
||
| private void updateItemMetadata(long extractedSize, long extractedGen) { |
There was a problem hiding this comment.
We can drop "this"
Use "this" only if the reference is ambigious.
| this.itemId = updatedItemId; | ||
| } | ||
|
|
||
| private volatile boolean metadataExtractionAttempted = false; |
There was a problem hiding this comment.
Please format. Move this to variable declaration section.
| fileSize = source.size(); | ||
| GcsItemInfo info = source.getItemInfo(); | ||
| if (info == null) { | ||
| return 0; |
There was a problem hiding this comment.
This change breaks footer prefetching.
In metadata extraction Failure Case:
The item info is never populated.
In metadata extraction Success Case:
The item info is populated only after first read. So the first read will still be served from gcs and not cached.
| GcsItemInfo.builder().setItemId(updatedItemId).setSize(extractedSize); | ||
| if (genToSet > 0) { | ||
| itemInfoBuilder.setContentGeneration(genToSet); | ||
| } else { |
There was a problem hiding this comment.
This is redundant. we should leave this unset. Please remove.
| Class<?> clazz = sdkChannel.getClass(); | ||
| while (clazz != null) { | ||
| for (String methodName : | ||
| new String[] { |
There was a problem hiding this comment.
Can you please create these as static constants instead.
| LOG.debug("Method {} not present on class {}", methodName, clazz.getName()); | ||
| } | ||
| } | ||
| for (String fieldName : new String[] {"storageObject", "blobInfo", "object", "result"}) { |
There was a problem hiding this comment.
can you please create these as static constants
| return true; | ||
| } | ||
|
|
||
| private void updateItemMetadata(long extractedSize, long extractedGen) { |
There was a problem hiding this comment.
nit: A bit confusing to read. Can we reorganize as follows.
extract generation
newline
itemId creation
newline
iteminfo creation
newline
set itemId and itemInfo
2 participants
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?
Implements reflective extraction of object size and generation from the GCS SDK ReadChannel after the first read.
Why?
This extraction allows optimizations like Parquet footer prefetching to run seamlessly after the first read even if file metadata is not provided by the user.
Checklist
feat(core): ...)Generated/Assisted by Agent? [Yes/No]