-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Multithreaded prefetcher option for newReadChannel #1414
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
Multithreaded prefetcher option for newReadChannel #1414
Conversation
Benefits: - the caller can run computation in between read calls, and data is being fetched in parallel with that computation. - the caller can issue many small sequential reads, and the system instead sends larger reads, which is more efficient.
|
Are we now requiring javadoc @return on all methods? It looks like this PR is failing on Travis because of (unrelated) missing Javadoc (?)
|
|
OK I've addressed most of the automatic review comments. Would like guidance as to what to throw for errors. |
|
Hopefully someone will be looking at this code soon? The code was initially submitted 12 days ago. |
|
I will take a look at it. We are very resource-constrained at the moment. |
garrettjonesgoogle
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.
I have made only a superficial pass so far. I will do another pass later where I look deeper.
| } | ||
| } | ||
| } catch (InterruptedException e) { | ||
| System.out.println("Timed out while waiting for channels to close."); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| try { | ||
| exec.awaitTermination(60, TimeUnit.SECONDS); | ||
| } catch (InterruptedException e) { | ||
| exec.shutdownNow(); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
Thank you, @garrettjonesgoogle, excellent! I was just worried that my PR has gone unnoticed, somehow. |
|
@garrettjonesgoogle I understand you're resource constrained. Could you review my shorter PRs before this one please? This may help us make the best of the situation. |
|
Thanks for the reviews on the smaller PRs! I'm going to close this one as I think we can do without it, and it'll free up some of your resources. We can reopen if we change our minds later. |
…1414) * chore(deps): update dependency com.google.cloud:libraries-bom to v26 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
🤖 I have created a release *beep* *boop* --- ### Updating meta-information for bleeding-edge SNAPSHOT release. --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
🤖 I have created a release *beep* *boop* --- ## [2.19.2](https://togithub.com/googleapis/java-datastore/compare/v2.19.1...v2.19.2) (2024-05-03) ### Bug Fixes * **deps:** Update the Java code generator (gapic-generator-java) to 2.39.0 ([#1406](https://togithub.com/googleapis/java-datastore/issues/1406)) ([b265fb3](https://togithub.com/googleapis/java-datastore/commit/b265fb3c3b8ebc972edbe5a7beae816379846dac)) ### Dependencies * Update dependency com.google.cloud:sdk-platform-java-config to v3.30.0 ([#1426](https://togithub.com/googleapis/java-datastore/issues/1426)) ([ac3a1c1](https://togithub.com/googleapis/java-datastore/commit/ac3a1c10f64c8338346f8fb39f4857f47e8fc639)) * Update dependency com.google.errorprone:error_prone_core to v2.27.0 ([#1411](https://togithub.com/googleapis/java-datastore/issues/1411)) ([a3f5a2c](https://togithub.com/googleapis/java-datastore/commit/a3f5a2c24bff408479541e08278e888cf3166727)) * Update dependency com.google.errorprone:error_prone_core to v2.27.1 ([#1421](https://togithub.com/googleapis/java-datastore/issues/1421)) ([48d7daf](https://togithub.com/googleapis/java-datastore/commit/48d7dafc0c7a49e95bf41d29865ac872b0de0faf)) * Update dependency com.google.guava:guava-testlib to v33.2.0-jre ([#1422](https://togithub.com/googleapis/java-datastore/issues/1422)) ([5a5dfdf](https://togithub.com/googleapis/java-datastore/commit/5a5dfdfb0855cf485b875ab071b79979d24f98dd)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
Benefits:
being fetched in parallel with that computation.
instead sends larger reads, which is more efficient.
NIO gets better performance when issuing parallel requests, as we already know from the ParallelCountBytes example. This pull request adds a "SeekableByteChannelPrefetcherOptions" that we can pass to newReadChannel to enable parallel prefetching without the NIO library, so the caller gets the benefits of parallel requests without having to change the client code at all.
The PR also updates the tests so they check the prefetching code path in addition to the normal, non-prefetching one.