-
Notifications
You must be signed in to change notification settings - Fork 260
Backport Bidi changes from master to 4.0.x #1553
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: branch-4.0.x
Are you sure you want to change the base?
Conversation
|
/gcbrun |
Summary of ChangesHello @Dhriti07, 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 backports a set of Bidi (Bi-directional) changes from the master branch to the 4.0.x branch. The primary goal is to introduce a new API for lexicographical listing of files and objects, alongside an upgrade of the underlying Hadoop version. Additionally, it refines the handling of vectored read operations by adding new metrics and improving resource management within the Bidi read channel, ensuring better performance and stability. 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 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 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
|
There was a problem hiding this 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 backports Bidirectional gRPC support and a new listStatusStartingFrom API from the master branch. The changes include dependency updates, implementation of the new API across different layers, and the addition of corresponding tests. The implementation looks solid, and the tests are comprehensive. I've included a few minor suggestions to improve documentation and release notes for clarity.
| 1. Upgrade hadoop versionto `3.4.2` | ||
| 2. Add bidi-support in connector | ||
|
|
||
| ### 4.0.x | ||
| 1. Add listStatusStartingFrom API. | ||
|
|
||
| 1. Add AUTO_RANDOM as new fadvise mode. | ||
|
|
||
| 1. Add getFileStatusWithHint() API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The release notes have a minor typo and the list formatting could be improved for readability. I'd suggest using sequential numbering for the list items and fixing the typo versionto.
| 1. Upgrade hadoop versionto `3.4.2` | |
| 2. Add bidi-support in connector | |
| ### 4.0.x | |
| 1. Add listStatusStartingFrom API. | |
| 1. Add AUTO_RANDOM as new fadvise mode. | |
| 1. Add getFileStatusWithHint() API | |
| 1. Upgrade hadoop version to `3.4.2` | |
| 2. Add bidi-support in connector | |
| 3. Add listStatusStartingFrom API. | |
| 4. Add AUTO_RANDOM as new fadvise mode. | |
| 5. Add getFileStatusWithHint() API |
| * Gets FileStatus of all files which are lexicographically greater than and equal the provided | ||
| * path. It filters out any directory objects present in underneath storage. | ||
| * | ||
| * <p>This is an experimental API can change without notice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * <p>Note: As GCS doesn't implement a file system, directory is also treated as an object (if | ||
| * it's been created). This APi filters out all those directory object and maintain the order of | ||
| * items. This APi strictly expects delimiter in listOptions to be not set. | ||
| * | ||
| * <p>Consider a bucket with objects: {@code o1}, {@code d1/}, {@code d1/o1}, {@code d1/o2} | ||
| * | ||
| * <ul> | ||
| * <li/>With {@code startOffset == "o1"} , we get: {@code o1} | ||
| * <li/>With {@code startOffset == "d1/"} , we get: {@code d1/o1} {@code d1/o2} | ||
| * <li/>With {@code startOffset == "d1/"o1} , we get: {@code d1/o1} {@code d1/o2} | ||
| * <p>This is an experimental API and can change without notice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Javadoc for this new method has a few typos and formatting issues that could be cleaned up for better readability. Specifically:
APishould beAPI.maintainshould bemaintains.<li/>should be<li>.- There's a typo in an example:
"d1/"o1should be"d1/o1". - The indentation of the final
<p>tag is off.
| * <p>Note: As GCS doesn't implement a file system, directory is also treated as an object (if | |
| * it's been created). This APi filters out all those directory object and maintain the order of | |
| * items. This APi strictly expects delimiter in listOptions to be not set. | |
| * | |
| * <p>Consider a bucket with objects: {@code o1}, {@code d1/}, {@code d1/o1}, {@code d1/o2} | |
| * | |
| * <ul> | |
| * <li/>With {@code startOffset == "o1"} , we get: {@code o1} | |
| * <li/>With {@code startOffset == "d1/"} , we get: {@code d1/o1} {@code d1/o2} | |
| * <li/>With {@code startOffset == "d1/"o1} , we get: {@code d1/o1} {@code d1/o2} | |
| * <p>This is an experimental API and can change without notice. | |
| * <p>Note: As GCS doesn't implement a file system, a directory is also treated as an object (if | |
| * it's been created). This API filters out all those directory objects and maintains the order of | |
| * items. This API strictly expects the delimiter in listOptions to not be set. | |
| * | |
| * <p>Consider a bucket with objects: {@code o1}, {@code d1/}, {@code d1/o1}, {@code d1/o2} | |
| * | |
| * <ul> | |
| * <li>With {@code startOffset == "o1"}, we get: {@code o1}</li> | |
| * <li>With {@code startOffset == "d1/"}, we get: {@code d1/o1}, {@code d1/o2}</li> | |
| * <li>With {@code startOffset == "d1/o1"}, we get: {@code d1/o1}, {@code d1/o2}</li> | |
| * </ul> | |
| * <p>This is an experimental API and can change without notice. |
| * <p>Note: As GCS doesn't implement a file system, directory is also treated as an object (if | ||
| * it's been created). This APi filters out all those directory object and maintain the order of | ||
| * items. This APi strictly expects delimiter in listOptions to be not set. | ||
| * | ||
| * <p>Consider a bucket with objects: {@code o1}, {@code d1/}, {@code d1/o1}, {@code d1/o2} | ||
| * | ||
| * <ul> | ||
| * <li/>With {@code startOffset == "o1"} , we get: {@code o1} | ||
| * <li/>With {@code startOffset == "d1/"} , we get: {@code d1/o1} {@code d1/o2} | ||
| * <li/>With {@code startOffset == "d1/"o1} , we get: {@code d1/o1} {@code d1/o2} | ||
| * <p>This is an experimental API and can change without notice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Javadoc for this new method has a few typos and formatting issues that could be cleaned up for better readability. Specifically:
APishould beAPI.maintainshould bemaintains.<li/>should be<li>.- There's a typo in an example:
"d1/"o1should be"d1/o1". - The indentation of the final
<p>tag is off.
| * <p>Note: As GCS doesn't implement a file system, directory is also treated as an object (if | |
| * it's been created). This APi filters out all those directory object and maintain the order of | |
| * items. This APi strictly expects delimiter in listOptions to be not set. | |
| * | |
| * <p>Consider a bucket with objects: {@code o1}, {@code d1/}, {@code d1/o1}, {@code d1/o2} | |
| * | |
| * <ul> | |
| * <li/>With {@code startOffset == "o1"} , we get: {@code o1} | |
| * <li/>With {@code startOffset == "d1/"} , we get: {@code d1/o1} {@code d1/o2} | |
| * <li/>With {@code startOffset == "d1/"o1} , we get: {@code d1/o1} {@code d1/o2} | |
| * <p>This is an experimental API and can change without notice. | |
| * <p>Note: As GCS doesn't implement a file system, a directory is also treated as an object (if | |
| * it's been created). This API filters out all those directory objects and maintains the order of | |
| * items. This API strictly expects the delimiter in listOptions to not be set. | |
| * | |
| * <p>Consider a bucket with objects: {@code o1}, {@code d1/}, {@code d1/o1}, {@code d1/o2} | |
| * | |
| * <ul> | |
| * <li>With {@code startOffset == "o1"}, we get: {@code o1}</li> | |
| * <li>With {@code startOffset == "d1/"}, we get: {@code d1/o1}, {@code d1/o2}</li> | |
| * <li>With {@code startOffset == "d1/o1"}, we get: {@code d1/o1}, {@code d1/o2}</li> | |
| * </ul> | |
| * <p>This is an experimental API and can change without notice. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## branch-4.0.x #1553 +/- ##
===============================================
Coverage ? 81.85%
Complexity ? 2416
===============================================
Files ? 126
Lines ? 10799
Branches ? 1300
===============================================
Hits ? 8839
Misses ? 1417
Partials ? 543
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dheerajsngh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the link of the Original PRs in the descritpion
|
Please hold onto this. We are planning to rebase. |
618d8b7 to
f2426d1
Compare
No description provided.