feat: add stream methods for Page
#1425
Conversation
product-auto-label
bot
added
size: m
|
Kudos, SonarCloud Quality Gate passed! |
JoeWang1127
added
the
owlbot:run
gcf-owl-bot
bot
removed
the
owlbot:run
|
Convert to draft for adding integration tests. |
This reverts commit 6fcfa9b.
JoeWang1127
added
the
owlbot:run
gcf-owl-bot
bot
removed
the
owlbot:run
|
Don't merge this yet. We want to control when to introduce this new feature. |
suztomo
added
the
do not merge
|
I'll merge main branch into this branch when it's ready to merge. |
|
Kudos, SonarCloud Quality Gate passed! |
| <className>com/google/api/gax/paging/Page</className> | ||
| <method>* stream*(*)</method> |
There was a problem hiding this comment.
It's strange that CLIRR is complaining the change. Adding default method in interface shouldn't cause a breaking changes.
There was a problem hiding this comment.
@JoeWang1127 Just in case, would you build this gax-java locally (with -SNAPSHOT version) and try to use in java-storage to see anything breaks? You ran some storage samples (https://github.com/JoeWang1127/storage-demo) that you used to understand the request.
There was a problem hiding this comment.
In Specifying ignored differences of Clirr Maven Plugin doc, 7012 has the following definition:
7012 (Method Added to Interface): className, method
So adding a method to interface, default or not, is a breaking change to this rule.
Also, there's 7013:
7013 (Abstract Method Added to Class): className, method
which is not complain.
There was a problem hiding this comment.
Clirr isn't aware of java 8 default methods. It is looking at the bytecode from the perspective of java7 binary compatibility. When adding a default method it is safe to specify an ignore rule.
There was a problem hiding this comment.
@BenWhitehead Thanks for the clarification.
|
@suztomo I've verified the change in the demo project (on branch |
JoeWang1127
changed the title
Page
|
Thank you. |
| * fetched whenever the elements of any particular page are exhausted. | ||
| */ | ||
| default Stream<ResourceT> streamAll() { | ||
| return StreamSupport.stream(iterateAll().spliterator(), false); |
There was a problem hiding this comment.
@BenWhitehead Would you elaborate what's not ideal? Parallel Stream.
There was a problem hiding this comment.
I think this is great for a first step of getting it on the api and saving our customers from having to do the same hoop jumping.
In the case of future improvements, right now this approach will not allow of parallel processing the same way a natively parallel friendly stream would. In all actuality, Page isn't fully parallelizable due to the fact that each page contains the nextPageToken necessary in order to fetch the next page. But, Page could be more parallel friendly where processing of the actual resources can happen on a thread separate from the thread performing the actual get.
There was a problem hiding this comment.
@JoeWang1127 Do you have question or comment about parallelism? (I don't think of an implementation as of now and don't think we need to implement it right now)
There was a problem hiding this comment.
I agree, parallelism does not need to block this PR and can be an additional improvement in the future.
There was a problem hiding this comment.
@suztomo I'm not understand about the separate thread in Ben's comment:
Page could be more parallel friendly where processing of the actual resources can happen on a thread separate from the thread performing the actual get.
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks for the reference.
I don't have a question about it.
suztomo
removed
the
do not merge
| @@ -133,6 +133,54 @@ public void pagedByPage() { | |||
| Truth.assertThat(requestCapture.getAllValues()).containsExactly(0, 2, 4).inOrder(); | |||
| } | |||
|
|
|||
| @Test | |||
| public void streamedByPage() { | |||
There was a problem hiding this comment.
Can we update the test names based on the best practices and a recent TotT?
|
[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed! |
|
[java_showcase_integration_tests] SonarCloud Quality Gate failed. |
|
[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed! |
Fixes #1426 ☕️