Catch throwable, ignore, retry


(Henrik Nordvik) #1

Hi,

A lot of places in ES are now catching Throwable. There are several commits
that have changed from catching Exception to catching Throwable.
For instance, TransportClientNodesService catches Throwable, ignores it and
retries the operation [1]. Some places it logs it as debug and continues.

Throwable includes OutOfMemoryError, StackOveflowError, LinkageError and so
on. These are usually unrecoverable problems, so catching Throwable is
usually not considered a good practice.

I think this looks very odd. Is this intentional? Are you really sure you
want to continue when you get those errors thrown, no matter what?

"An Error is a subclass of Throwable that indicates serious problems that a
reasonable application should not try to catch."

Henrik

[1]
https://github.com/elasticsearch/elasticsearch/blob/master/src/main/java/org/elasticsearch/client/transport/TransportClientNodesService.java#L267

--
You received this message because you are subscribed to the Google Groups "elasticsearch" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elasticsearch+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elasticsearch/47ea9472-0606-4107-bc9c-a2dcd85c8d29%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


(Jörg Prante) #2

I agree that programs that work in an environment with safe and predictable
resource consumption should always follow an all-checked exception approach.

In reality, ES threads that encounter OOM or suddenly fall short on
resources like network connections for a few milliseconds can often
survive, because there are so many threads that can temporarily exceed
limits. In case of TransportClientNodesService, it is better to let ES
retry for a certain amount, otherwise the failure would propagate to the
user app, and ES JVM would exit with failure. Imagine a feeder app that
would always exit in case of too large docs, or restarting nodes, instead
of trying to continue...

Jörg

On Mon, Mar 24, 2014 at 9:08 PM, Henrik Nordvik henrikno@gmail.com wrote:

Hi,

A lot of places in ES are now catching Throwable. There are several
commits that have changed from catching Exception to catching Throwable.
For instance, TransportClientNodesService catches Throwable, ignores it
and retries the operation [1]. Some places it logs it as debug and
continues.

Throwable includes OutOfMemoryError, StackOveflowError, LinkageError and
so on. These are usually unrecoverable problems, so catching Throwable is
usually not considered a good practice.

I think this looks very odd. Is this intentional? Are you really sure you
want to continue when you get those errors thrown, no matter what?

"An Error is a subclass of Throwable that indicates serious problems that
a reasonable application should not try to catch."

Henrik

[1]
https://github.com/elasticsearch/elasticsearch/blob/master/src/main/java/org/elasticsearch/client/transport/TransportClientNodesService.java#L267

--
You received this message because you are subscribed to the Google Groups
"elasticsearch" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to elasticsearch+unsubscribe@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/elasticsearch/47ea9472-0606-4107-bc9c-a2dcd85c8d29%40googlegroups.comhttps://groups.google.com/d/msgid/elasticsearch/47ea9472-0606-4107-bc9c-a2dcd85c8d29%40googlegroups.com?utm_medium=email&utm_source=footer
.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "elasticsearch" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elasticsearch+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elasticsearch/CAKdsXoEwd3Zw5sCQ9TkcRO5gq-t7cg1L3cL%2B-Nexsf_fNAjFNw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.


(Henrik Nordvik) #3

I think you misunderstood me. I'm not talking about checked vs unchecked
here. Catching Throwable vs catching Exception is a different beast.

Specifically in the example I showed you:

try {
callback.doWithNode(nodes.get((index + i) % nodes.size()), this);
} catch (Throwable thisThrowableIsAlwaysIgnored) {
// retry the next one...
onFailure(e);
}

Also, this is in the client, which means you are hiding potentially
important exceptions from clients.
We found one similar catch throwable in the JDK, which meant that we did
not get any clue as to why our program did not work. We actual had to patch
the JDK to get the original error printed.

Henrik

On Monday, March 24, 2014 10:19:17 PM UTC+1, Jörg Prante wrote:

I agree that programs that work in an environment with safe and
predictable resource consumption should always follow an all-checked
exception approach.

In reality, ES threads that encounter OOM or suddenly fall short on
resources like network connections for a few milliseconds can often
survive, because there are so many threads that can temporarily exceed
limits. In case of TransportClientNodesService, it is better to let ES
retry for a certain amount, otherwise the failure would propagate to the
user app, and ES JVM would exit with failure. Imagine a feeder app that
would always exit in case of too large docs, or restarting nodes, instead
of trying to continue...

Jörg

On Mon, Mar 24, 2014 at 9:08 PM, Henrik Nordvik <henr...@gmail.com<javascript:>

wrote:

Hi,

A lot of places in ES are now catching Throwable. There are several
commits that have changed from catching Exception to catching Throwable.
For instance, TransportClientNodesService catches Throwable, ignores it
and retries the operation [1]. Some places it logs it as debug and
continues.

Throwable includes OutOfMemoryError, StackOveflowError, LinkageError and
so on. These are usually unrecoverable problems, so catching Throwable is
usually not considered a good practice.

I think this looks very odd. Is this intentional? Are you really sure you
want to continue when you get those errors thrown, no matter what?

"An Error is a subclass of Throwable that indicates serious problems that
a reasonable application should not try to catch."

Henrik

[1]
https://github.com/elasticsearch/elasticsearch/blob/master/src/main/java/org/elasticsearch/client/transport/TransportClientNodesService.java#L267

--
You received this message because you are subscribed to the Google Groups
"elasticsearch" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to elasticsearc...@googlegroups.com <javascript:>.
To view this discussion on the web visit
https://groups.google.com/d/msgid/elasticsearch/47ea9472-0606-4107-bc9c-a2dcd85c8d29%40googlegroups.comhttps://groups.google.com/d/msgid/elasticsearch/47ea9472-0606-4107-bc9c-a2dcd85c8d29%40googlegroups.com?utm_medium=email&utm_source=footer
.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "elasticsearch" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elasticsearch+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elasticsearch/8c704742-8fa0-4c98-a8c5-be2a3834917b%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


(system) #4