Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: add stream methods for
Page
#1425feat: add stream methods for
Page
#1425Changes from 21 commits
ca6f08d
3a08ea9
99adcac
00e847e
9688e3d
647d333
580c6ef
206c46f
6fcfa9b
9fbfa93
8206560
d214420
7b257d1
9694bb1
82a9d72
a7d26fe
89128c1
974712d
3da41ea
df4c7e7
7682f22
654fedb
069d9a5
17a0a9c
f133040
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Specifying ignored differences of Clirr Maven Plugin doc, 7012 has the following definition:
So adding a method to interface, default or not, is a breaking change to this rule.
Also, there's 7013:
which is not complain.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BenWhitehead Thanks for the clarification.
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.
@BenWhitehead Would you elaborate what's not ideal? Parallel Stream.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@suztomo I'm not understand about the separate thread in Ben's 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.
This https://docs.oracle.com/javase/tutorial/collections/streams/parallelism.html#executing_streams_in_parallel
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.
Thanks for the reference.
I don't have a question about it.
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.
Can we update the test names based on the best practices and a recent TotT?
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.
done