-
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
Initian design for OTHER status[DRAFT] #4015
Initian design for OTHER status[DRAFT] #4015
Conversation
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 suggest consideration towards putting the 'other status' onto PollResponse itself, and then returning a lot of the code to how it was previously. I don't presently see a benefit to having this extra layer of API?
* which must never return {@code null}, and which must always have a non-null {@link com.azure.core.util.polling.PollResponse.OperationStatus}. | ||
*{@link Mono} returned from poll operation should never return {@link Mono#error(Throwable)}.If any unexpected scenario happens in poll operation, | ||
* @param pollInterval Not-null and greater than zero poll interval. | ||
* @param pollOperation The polling operation to be called by the {@link Poller} instance. This is a callback into the client library, |
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.
Don't do this kind of alignment
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 auto format in Intellij which I should turn off.
//observeState.add(OperationStatus.State.FAILED); | ||
this.fluxHandle = this.fluxHandle.filterWhen(tPollResponse -> matchesState(tPollResponse, observeState, observeOtherStates)); | ||
return this.fluxHandle; | ||
} |
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 this API is necessary. The user can implement the filter on their side.
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.
That's true , makes poller simpler.
Be sure to refer back to the issue so context can be track - this PR is for issue #3930 |
*/ | ||
public PollResponse(OperationStatus status, T value, Duration retryAfter, Map<Object, Object> properties) { | ||
public PollResponse(OperationStatus status, String otherStatus, T value, Duration retryAfter, Map<Object, Object> properties) { |
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 would simplify the constructor - if the user has supplied an 'otherStatus', you don't need them to supply an operation status, you can automatically make that OTHER.
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.
Simplify the constructors, and be sure to run checkstyle / spotbugs.
This is for design discussion for OTHER status. Test will fail since they are not updated.