-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Properly implement observable feature in javanica using HystrixObservableCommand #948
Comments
@dmgcodevil any thoughts? I might take a shot at a PR if you have any suggestions. |
I'm working on it, will push my changes soon
|
Awsome! I saw a bunch of good stuff in 1.4.20. Keep up the good work! |
@mattrjacobs what method should I use by default to execute HystrixObservableCommand: observe() or toObservable() ? |
|
then it's better to add extra parameter in annotation to switch between two modes, thanks |
I'm just thinking about how to implement observable collapser feature in javanica and have noticed that HystrixObservableCollapser provides some extra methods that return functions used to map BatchReturnType to ResponseType and etcetera. For my knowlege regular HystrixCollapser doesn't have such feature and when I was implementing regular collapser feature in javanica I added some logic that imposes restrictions on BatchReturnType and ResponseType, intrinsically both types must be the same otherwise javanica complains. On the one hand I don't want to deny this ability and want to allow end users to use those functions. There are an ideas that came to my mind, that is add extra parameter in HystrixCollapser annotation to specify class of a function that should be used or specify function name likewise fallbacks specified. But both approaches have disadvantages: private static class UserToName implements Func1<User, String> {
@Override
public String call(User user) {
return user.getName();
}
} Collapser @HystrixCollapser(batchReturnTypeToResponseTypeMapper = UserToName.class) From my point of view it will lead to tedious code and deprives of using lamdas, I think users will complain about it. Though the second case is better it can lead to error prone code because java compiler can't check correctnes of return types at compile time. Though Javanica makes some efforts to check types in runtime before Hystrix command is created and executed, this applies for fallbacks methods and collapsers, but it would be tricky to be safe in the case of observable collapser because an user can use parametrized functions and there is more than one function can be declared. But it's feasable though. Summarizing that, there are two ways, add extra parameters to allow users set functions types in annotation or add an abitlity to specify just raw functions names similary to fallbacks or impose restrictions on users by default and desibale the ability to use those mapper functions, it results that BatchReturnType and ResponseType must be same. @mattrjacobs, @spencergibb what do you think ? |
@dmgcodevil javanica users are already used to the fallback method. I hesitate using classes that get created (ie |
@spencergibb by finalising which approach is preferable we can move forward, specifying fallback as method name in annotation makes sense because most likely fallback method will be in same class where is a command. There is one thing concerns me is that type checking, because either an ide or Java compiler would not catch such errors, in the case of fallback it's pretty much simple to catch by looking at code. There are 4 functions that can be declared for observable collapser and a developer can easily make a mistake during development or refactoring, so I'm thinking about more durable way. |
@dmgcodevil Until there is a fix for this issue is it possible to use Hystrix Javanica as described https://github.com/Netflix/Hystrix/tree/master/hystrix-contrib/hystrix-javanica#reactive-execution |
@andrewe123 yes it was implemented and released, just check out latest version, but collapser feature isn't supported yet though. |
@dmgcodevil Which version? I'm on 1.4.20 and I'm getting the same exception when testing something like this https://github.com/Netflix/Hystrix/tree/master/hystrix-contrib/hystrix-javanica#reactive-execution my test looks like
|
I guess it's wrong version because I removed ObservableResult
|
@dmgcodevil I tested with the latest available release, 1.4.21. The exception is still throw and ObservableResult is still in the jar. Is the fix planned for a future release? |
@dmgcodevil did you ever get this working? I'm having the same issue as described in #987 (comment) |
Yes, I'm sure that I removed ObservableResult. What version are you using +
|
@dmgcodevil 1.4.21 with Brixton.M4. My code looked just like your code. |
Closing due to inactivity. Please re-open if there's more to discuss. |
@dmgcodevil any news about supporting Observables by collapser in javanica? |
@p-pekala I think it's feasible so answer is yes. I don't remember if I faced any obstacles to implement it in first place but it appears to be possible in general. Probably it will be the next thing that I will implement in the near future. Thanks |
@dmgcodevil +1 for collapsible Observables |
Created from #729
The text was updated successfully, but these errors were encountered: