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

Add toObservable() to the common HystrixExecutable interface #312

Closed

Conversation

bbaetz
Copy link

@bbaetz bbaetz commented Sep 9, 2014

All of the implementers of HystrixExecutable implement toObservable() and toObservable(Scheduler observeOn)

This patch adds those methods to the common interface, and also adds @OverRide annotations to appropriate places.

Use case is that I have a factory method that may return a HystrixCollapser or a HystrixCommand, and want to be able to call .toObservable on the result. (I'm still doing some designs on this and may not use it that way, but hopefully this technically-breaking change can make 0.40....)

@cloudbees-pull-request-builder

Hystrix-pull-requests #152 SUCCESS
This pull request looks good

@benjchristensen
Copy link
Contributor

This will break anybody who implements this interface. No one should really be doing that, but are we okay with a possible breaking change?

@bbaetz
Copy link
Author

bbaetz commented Sep 27, 2014

This will break anybody who implements this interface.

Yep.

No one should really be doing that

So the reason that I wanted this is because I am implementing this interface, as a wrapper/delegate around a set of commands that basically do:

  • get from cache
  • if its not there, get from the real service, then asyncrhonously write back to the cache.

This does mean that I could just create my own interface that includes HystrixExecutable+toObservable(), and I'm not sure that I acually need the lazy toObservable, so maybe this isn't worth putting upstream?

@benjchristensen
Copy link
Contributor

Then you are an example of someone this would break. If a 1.x release came out that causes your code to break, wouldn't you consider that a problem since it wasn't a 2.x major version bump?

This is an example of why Java interfaces are troublesome in public APIs prior to Java 8 default methods.

Currently in the master branch I have a HystrixObservable interface that includes observe and toObservable that is in addition to HystrixExecutable. I'd appreciate your review of the API in #321.

@bbaetz
Copy link
Author

bbaetz commented Sep 27, 2014

Yeah, fair enough. I'll comment on the other ticket

@bbaetz bbaetz closed this Sep 27, 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