Skip to content
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

Refactoring of load balancer execution API and add ExecutionListener #165

Merged
merged 21 commits into from
Sep 16, 2014

Conversation

allenxwang
Copy link

@cloudbees-pull-request-builder

ribbon-pull-requests #176 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

ribbon-pull-requests #177 SUCCESS
This pull request looks good

}

public CommandBuilder<T> withListeners(List<? extends ExecutionListener<?, T>> listeners) {
this.listeners = listeners;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can withListeners be called multiple times? Might want to accumulate the listeners instead of replacing previous ones.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the parameter is already a Collection, I feel it will seem strange if we do further accumulation for the with() method in a builder. At least it does not seem intuitive to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had the same thing in governator and eventually had to add a method called withAdditionalListeners for backwards compatibility. This caused a lot of ambiguity with sequences of withAdditionalListeners mixed with withListeners.

… to LoadBalancingHttpClient to be consistent with other namings.
@cloudbees-pull-request-builder

ribbon-pull-requests #178 SUCCESS
This pull request looks good

…Invoker. Added new APIs in ExecutionContextListenerInvoker to invoke listeners with passed in context.
@cloudbees-pull-request-builder

ribbon-pull-requests #181 FAILURE
Looks like there's a problem with this pull request

@cloudbees-pull-request-builder

ribbon-pull-requests #182 SUCCESS
This pull request looks good

allenxwang pushed a commit that referenced this pull request Sep 16, 2014
Refactoring of load balancer execution API and add ExecutionListener
@allenxwang allenxwang merged commit b31677e into Netflix:master Sep 16, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants