spring-cloud-commons icon indicating copy to clipboard operation
spring-cloud-commons copied to clipboard

Don't throw IllegalStateException when no instances are available

Open OrangeDog opened this issue 8 years ago • 11 comments

Once it comes out of a RestTemplate it should be a RestClientException, preferably a ResourceAccessException, so it can be handled consistently.

At the moment it is annoying to deal with.

OrangeDog avatar Feb 24 '17 15:02 OrangeDog

Not sure I agree. From RestClientException

"Base class for exceptions thrown by {@link RestTemplate} whenever it encounters client-side HTTP errors."

I definitely wouldn't use ResourceAccessException

Exception thrown when an I/O error occurs

Neither one of those things happens when no instances are available.

How often do you deal with no instances available?

spencergibb avatar Feb 24 '17 15:02 spencergibb

No instances being available is a client-side HTTP error. If you chose one you thought was available but it turned out not to be, a ResourceAccessException would be thrown. Pre-knowledge of this shouldn't throw a generic, unexpected exception.

How often do you deal with no instances available?

That's not really relevant. When it does happen I need to be able to deal with it appropriately, which already happens for non-load-balanced RestTemplates.

OrangeDog avatar Feb 24 '17 16:02 OrangeDog

I can be convinced of RestClientException, but I don't think ResourceAccessException is appropriate.

spencergibb avatar Feb 24 '17 16:02 spencergibb

The key term is "HTTP errors". Not having any instances available doesnt seem like an HTTP error to me.

ryanjbaxter avatar Feb 27 '17 15:02 ryanjbaxter

@ryanjbaxter have you looked at how Spring already uses it? As I said, you already get RestClientException if no instances are available - it's only in this specific case when Spring Cloud is involved that it's changed into an IllegalStateException.

OrangeDog avatar Feb 27 '17 15:02 OrangeDog

Sorry I was consumed by the terminology "instances". When using a plain RestTemplate (without it being load balanced) I wouldnt think that would apply.

ryanjbaxter avatar Feb 27 '17 15:02 ryanjbaxter

Well in a plain RestTemplate if the only instance is down then you get RestClientException. In a @LoadBalanced RestTemplate if all the instances are down but not currently marked, you get RestClientException. If anything else goes wrong during any kind of RestTemplate execution, you get RestClientException.

In this one specific case you get IllegalStateException when it's clearly not even an illegal state: it can happen as a matter of course.

OrangeDog avatar Feb 27 '17 15:02 OrangeDog

In the 1st case, you are actually trying to create a connection (there's no notion of instances). In the 2nd it never gets to that point. We've marked it for team discussion. The arguing of semantics here isn't helping.

spencergibb avatar Feb 27 '17 15:02 spencergibb

I'm not arguing semantics, I'm pointing out practical reasons why this should be changed.

OrangeDog avatar Feb 27 '17 15:02 OrangeDog

Has this been discussed yet within the spring-cloud team? I also believe a more specialised runtime exception would be appropriate here. ISE is too general for this case.

rkettelerij avatar Nov 13 '18 13:11 rkettelerij

There are two steps a new exception and catching and exception in the LoadBalancerInterceptor

spencergibb avatar Feb 07 '19 16:02 spencergibb