defer the Creation of ISE, to when an exception needs to be created

[tootedom]

Hi there,

I noticed that an ISE is always created in the doOnComplete, even if the exception case does not occur, which incurs a slight performance hit from the Exception construction and it's initialisation of the stack trace (fillInStackTrace()):

• https://github.com/Netflix/Hystrix/blob/master/hystrix-core/src/main/java/com/netflix/hystrix/collapser/RequestBatch.java#L146

The PR moves this to the passing in of a String that represents the ISE Exception Message if/when it occurs.

You could tackle it by having a custom ISE that overloads the fillInStackTrace() method to return null, and have a custom method that calls super.fillInStackTrace() if/when required.

cheers
/dom

[cloudbees-pull-request-builder]

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

[benjchristensen]

Looks like a valid thing to optimize, however, won't this now cause the ISE to be created N times when an error does occur?

[tootedom]

Hi there,

Sorry, maybe I'm not following. I'm not sure how it's going to create multiple ISE's, N times when an error occurs. The method is just calling the original setExceptionIfResponseNotReceived(Exception e) method. It will definitely be a little less efficient in the Exception case, as it'll do the checks again for the response being set: if (!r.isResponseSet() && r.getException() == null) { and the entering of the while loop.

Or do you mean from the fact that the ISE is now created inside the For, and as a result the ISE is created multiple times? for (CollapsedRequest<ResponseType, RequestArgumentType> request : shardRequests). Which, yes, would be more inefficient; but I'm guessing the edge case for the comment, is an exception; and you wouldn't normally see the exception created?

// check that all requests had setResponse or setException invoked in case 'mapResponseToRequests' was implemented poorly

Bit of a chicken and egg for sure. Optimising for the more common case would then be the best option; unless there's another approach.

Could the setIllegalStateExceptionIfResponseNotReceived return the ISE? and the be reused in the for.... Seems a bit messy though to check on null in the for loop.... hmmmm

[benjchristensen]

I'm not sure how it's going to create multiple ISE's

It's in a for-loop.

Optimising for the more common case would then be the best option

If it was the only choice, sure. But since this is about optimizing, let's make it so the ISE is only created once if needed, and then if needed more than once, is reused.

[tootedom]

If it was the only choice, sure. But since this is about optimizing,
let's make it so the ISE is only created once if needed, and then if
needed more than once, is reused.

Perhaps the update:
tootedom@7021050

[cloudbees-pull-request-builder]

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