BatchHystrixCommand has no sence and doesn't collapse requests

[anton-antonov]

BatchHystrixCommand calls underlying action as many times as it's requested and thus there is no benefit of collapse requests feature at all.

Actual processing Optional.of(fallbackEnabled ? processWithFallback(args) : process(args)) should be done only once for same set of request args.

@Override
    protected List<Optional<Object>> run() throws Exception {
        List<Optional<Object>> response = Lists.newArrayList();
        for (HystrixCollapser.CollapsedRequest<Object, Object> request : getCollapsedRequests()) {
            final Object[] args = (Object[]) request.getArgument();
            try {
                response.add(
                     //this should be called only once for several equals args arrays, 
                     //otherwise underlying command action will be called as many times as many it was requested
                      Optional.of(fallbackEnabled ? processWithFallback(args) : process(args))
                );
            } catch (Exception ex) {
                request.setException(ex);
                response.add(Optional.absent());
            }
        }
        return response;
    }

[anton-antonov]

There should be a convenient way to override reduce logic assume should be a new annotation to mark
batch method with Collection<CollapsedRequest<ResponseType, RequestArgumentType>> requests

[mattrjacobs]

This looks like something in the hystrix-javanica module. /cc @dmgcodevil @sawano

[dmgcodevil]

Hi @anton-antonov, probably I didn't get it completely when was implementing collapser feature, therefore lets refresh it and figure out a gaps in my understanding in order to fix it.

A method annotated with HystrixCollapser is invoked within CommandCollapser. CommandCollapser uses a method arguments (it's an array) to create collapsed requests and afterwards creates BatchHystrixCommand to make underlying calls. BatchHystrixCommand is invoked once for collapsed requests that prepared by CommandCollapser. Action is invoked as many times as much elements in Collection<HystrixCollapser.CollapsedRequest<Object, Object>> and it's used to invoke a method but BatchHystrixCommand is invoked once per collection of CollapsedRequest objects. In my opinion there is no incorrect behavior, isn't ?

 final Object[] args = (Object[]) request.getArgument();

here args is args of an annotated method.
About reduce logic. I think it's ok to have such facility in order to override default reduce logic. We can add new property to HystrixCollapser annotation. It can be method name with same signature as for mapResponseToRequests :

        @HystrixCommand
        @HystrixCollapser(reducer="getUserReducer")

        public User getUser(String id, String name) {
            return new User(id, name + id);
        }

private void getUserReducer(List<Optional<Object>> batchResponse,
                                         Collection<CollapsedRequest<Object, Object>> collapsedRequests);

Or we can add interface Reducer with single method and specify the type of certain implementation in @HystrixCollapser.

WDYT ?

[anton-antonov]

Hi @dmgcodevil, thanks for your response.

Yes, BatchHystrixCommand is invoked once per collapsed requests collection but underlying CommandAction performed for each request in this collapsed collection

Let's say I have a command like this

@HystrixCommand()
@HystrixCollapser()
public String getValue(Long id) {
    //here I made a heavy backend call to remote service
    return this.client.getValue(id);
}

then I perform the following call with thousands of threads (1000 requests/sec)

myCommand.getValue(10L);

Notice that all 1000 requests are calling the same ID parameter and I expect that backend service will be called only once or at least the less number of times then client does.

More over current implementation reduces all benefits at all, look:
Without @HystrixCollapser I'd have 1000 parallel calls to my backend service and probably they'd responded in a same timeout let say 20ms
With @HystrixCollapser they all are collapsed to a single thread and called one-by-one, thus as a result I'll get 1000 sequential identical calls to backend service with response time = 20ms * 1000 = 20sec.

With custom reducing logic it should be possible to utilise backend service batching facilities if backend service has something like this:

public Map<Long, String> getValues(List<Long> ids);

[dmgcodevil]

Hi @anton-antonov, thanks for the response
If you want to reduce the count of backend calls processing with same parameters you can use Hystrix cache feature. for example:

        @CacheResult
        @HystrixCommand
        public String getValue(@CacheKey Long id) {
            return this.client.getValue(id);
        }

I got your point about collapser, but I don't have any ideas how to implement it using annotations.
Because if you use pure Hystrix then you have two entry points:a command parameters and backend service parameters, in this case your command expects one parameter and backend service expects multiple parameters, thus you are able to manage the case that you've mentioned above. But I don't know how to implement it using annotation, because all that you have is method of backend service. We need some ability to represent to method: the first works with single parameters and another one works with a collection of "single" params. WDYT ?

[anton-antonov]

Thanks, @dmgcodevil.

Point here is just about collapsing exact identical backend calls and it is possible even with annotations.
Just combine same CollapsedRequest by their request.getArgument(). Or multiply results of a single underlying call to requests with the same request.getArgument() value.

@CacheResult works only in Request scope. But collapsers can work at GLOBAL scope. That's actually the reason why I can't use @CacheResult annotations.

As a conclusion:
BatchHystrixCommand does not Collapse underlying backend calls as it stands. And usage of @HystrixCollapser should be avoided. As a w/o there is need to implement HystrixCollapser class and follow the pure Hystrix tutorial for command collapsing

[dmgcodevil]

Thus the collapser will play same role as Hystrix cache, it just will deduplicate same parameters and that's all, right ? @mattrjacobs what do you think about it ?

[mattrjacobs]

Stepping back, the reason that collapsers exist is to enable batching that is performed on behalf of the caller without the caller being forced to do any batching-specific logic. This is different from Hystrix request caching.

• Hystrix request caching allows different command instances with the same arguments to only make 1 backend call, then cache the response for all such invocations.
• Hystrix request collapsing allows different command instances with different arguments to get batched into a single command call (which is batch-aware) and reduce network traffic.

So the 2 major pieces of machinery needed for a collapser implementation are:

1. A way to convert n non-batched commands into a single batch command. In hystrix-core, this is abstract HystrixCommand<BatchReturnType> createCommand(Collection<CollapsedRequest<ResponseType, RequestArgumentType>> requests).
2. A way to demultiplex a batch response into n single-valued responses for the initial non-batched commands. In hystrix-core, this is abstract void mapResponseToRequests(BatchReturnType batchResponse, Collection<CollapsedRequest<ResponseType, RequestArgumentType>> requests).

If you are able to represent those as annotations, then you should be able to achieve your goal. I'm not familiar enough with the hystrix-javanica design space to offer a detailed prescription, but hopefully this is enough to get you headed the right direction.

[dmgcodevil]

Thanks @mattrjacobs. Actually it seems like I got it almost correctly originally but implemented it a bit incorrectly. Now I see how it can be implemented:
for instance:

@HystrixCollapser(command="getByValues")
      String getByValue(Integer val){
      return null; // this is stab, this line shouldn't invoked, instead of it the batch command will be executed
}

@HystrixCommand(reducer= CustomReducer.class) // reducer is optional field because it must provide default implementation
      List<String> getByValues(List<Integer> values){
       // network call
}

interface Reducer<ResponseType, RequestArgumentType> {
      ResponseType reduce(RequestArgumentType requestArg);
}

class CustomReducer implements Reducer<String, Integer> {
      String reduce(Integer requestArg) {
      return "ValueForKey: " + requestArg;
  }
}

What do you think? (@anton-antonov, @mattrjacobs)

[mattrjacobs]

This looks generally on the right path. In hystrix-core, the demultiplexing (you've named it reducer is a property of the collapser, not the command. That's because it's completely valid to call the command in a batch way programmatically, outside of any auto-collapsing, if that's how you want to use it. In that case, you don't need to demultiplex the response. So, IMO, both the batching and demultiplexing semantically belong on the collapser, not the command.

[dmgcodevil]

Thanks @mattrjacobs. In my opinion it's not reducer it's more like mapper. Reducing logic wouldn't be changed, it will be implemented as in core:

 @Override
    protected void mapResponseToRequests(List<String> batchResponse, Collection<CollapsedRequest<String, Integer>> requests) {
        int count = 0;
        for (CollapsedRequest<String, Integer> request : requests) {
            request.setResponse(batchResponse.get(count++));
        }
    }

Mapper gives the ability to manage mapping logic at reducing step and it's just additional feature, but I'm not sure that it gives significant advantages, but probably will be useful in some cases.

Ok, so lets summarize it:
Collapser:

@HystrixCollapser(command="getByValues", mapper= CustomMapper.class) // mapper is optional
      String getByValue(Integer val){
      return null; // this is stab, this line shouldn't invoked, instead of it the batch command will be executed
}

Command:

@HystrixCommand
      List<String> getByValues(List<Integer> values){
       // network call
}

interface Mapper<ResponseType, RequestArgumentType> {
      ResponseType map(RequestArgumentType requestArg);
}

If it's ok then I will start implementation. Also please assign this issue to me. Thanks.

[mattrjacobs]

As far as I can tell, that looks right to me.

[mattrjacobs]

Tried to assign to you, but that only seems to work for people in the Netflix Github team. I'll try to get that working

[dmgcodevil]

Please see PL

[dmgcodevil]

@mattrjacobs did you change code of projects to use jdk 6 ?

[mattrjacobs]

Yes, JDK6 now, as of Hystrix 1.4.3. See #731

[mariuszluciow]

@dmgcodevil I compiled and tested yours fork. Everything seems to work just fine. Is there any chance to include support for void methods? For example something like that:

    @HystrixCollapser(commandKey = "storeUsers")
    public void storeUser(String id) {
    }

    @HystrixCommand
    public void storeUsers(List<String> ids) {
    }

[dmgcodevil]

Hi @MariuszNET, this changes were merged to upstream one week ago and I guess it's available in hystrix 4.x.x
I even don't know about void methods, technically it's possible, but have never used hystrix commands to store something. If you really need such facility then you need to create separate issue. But unfortunately at the current moment you need to find some workaround. Basically you can just use the given list in return statement:

   @HystrixCollapser(commandKey = "storeUsers")
    public Future<String> storeUser(String id) {
           return null;
    }

    @HystrixCommand
    public List<String> storeUsers(List<String> ids) {
           // put your logic here
           return ids;
    }

Or create a wrapper for your code if you cant change API.
Thanks,
dmgcodevil

[mariuszluciow]

@dmgcodevil Thanks for info about merge - I missed this. Version 1.4.4 contains changes and works well.
With workaround everything is ok. We wanna use this on void methods for sending batch with notifications to other service, instead of sending one on each time.

Thanks,
mariusznet