bbc / alephant-broker

Brokers requests for alephant components
MIT License
4 stars 3 forks source link

[Review] Enable loading components in parallel (batch) #22

Closed stevenjack closed 9 years ago

stevenjack commented 9 years ago

oxjjfnv - imgur

Problem

After testing the batch loading endpoint, it became apparent that the components are still loaded async and not in parallel in the response.

After having a look through the code, I realised that even though we're generating the json using pmap, we're doing the actual loading in the factory: https://github.com/BBC-News/alephant-broker/blob/master/lib/alephant/broker/component_factory.rb#L19.

Solution

The changes I've made I tested in production and they worked in terms of doing the actual loading in parallel and brought the response time down dramatically.

I know there could be some knock on effects from these changes which is evident from the failing tests, but the in principal it works we just need to have a re-think about the error catching in the factory I think.

Edit by @revett

.pmap was being used in the wrong place (BatchResponse), where the components had already been loaded (sequentially). I simply moved it to the BatchRequest object so that the ComponentFactory could initialize components in parallel. It handles errors (e.g. ContentNotFound) in exactly the same way. :surfer:

Integralist commented 9 years ago

@revett @darnould can you pass your eyes over this please. The fundamental changes @stevenjack has made makes sense and looks fine (with the exception of still needing to remove the commented code) but there may be some subtleties in the changes that I would not be aware of (as you guys have done a lot of work with the broker recently).

stevenjack commented 9 years ago

Just writing a description for this to give it some context.

revett commented 9 years ago

@stevenjack good spot bro :eyes:

As discussed, we can move the .pmap functionality from the Response object to the Request object - see RequestBatch #L29.

revett commented 9 years ago

Requesting 3 different components in a batch request (local):

Before: 1600-1800ms After: 300-600ms

revett commented 9 years ago

@stevenjack I reverted your commit and changed .pmap to be used in BatchRequest. Definitely now loading components in parallel (see previous comment) and handling errors as designed.

darnould commented 9 years ago

:+1: Awesome.

stevenjack commented 9 years ago

:cake: