-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[#5243] Adding progress reporter to bulk operations #5730
Conversation
* that this type be used in conjunction with {@link ProgressReporter#addProgressReporting(Flux, IProgressReceiver)}. | ||
*/ | ||
interface IProgressReceiver { | ||
public interface IProgressReceiver { |
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'm not a fan of interfaces starting with I
. I would much prefer to see this called ProgressReceiver
.
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.
do we have any info about this in out guidance? for the preferred prefix-naming stuff?
I see a few interfaces doing the I..
naming within the project
Might be a good idea to have a docs with any agreement on this.
I don't really have any specific preference regarding to this. I am fine updating 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.
Yes, I will update spec to say to not do this.
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 don't think there is any public API with the I prefix. If you do find any, please let me know.
* @return A {@code Flux} that emits the same data as the source but calls a callback to report the total amount of | ||
* data emitted so far. | ||
*/ | ||
public static Flux<ByteBuffer> addParallelProgressReporting(Flux<ByteBuffer> data, |
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 don't think we want this method public. It's really for our own use when we handle parallel uploads
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.
we are using it from com.azure.storage.blob.specialized
(BlobAsyncClientBase#download) and from com.azure.storage.blob
(BlobAsyncClient:upload).
We would need to make some extra refactoring to have BlobAsyncClientBase
in same package and then get rid of the public
access
fixes: #5243
Wraps implementation for IProgressReceiver to trigger
reportProgress
on each upload/download step