move logger.trace() outside of try statement

[zztv]

While using TransportClient in application with older log4j version 1.2.8 it is possible to get very misleading NoNodeAvailableException (see at the very bottom of this message):

The reason is that log4j doesn't have method trace() and call to logger.trace() is placed inside try/catch(Throwable) below. It results in exception being thrown during execution of logger.trace() which later lead to NoNodeAvailable exception down the road.

Thanks,
Igor.

protected ImmutableList validateNewNodes(Set nodes) {
for (Iterator it = nodes.iterator(); it.hasNext(); ) {
DiscoveryNode node = it.next();
if (!transportService.nodeConnected(node)) {
try {
logger.trace("connecting to node [{}]", node);
transportService.connectToNode(node);
} catch (Throwable e) {
it.remove();
logger.debug("failed to connect to discovered node [" + node + "]", e);
}

May 8 11:15:28 9.21.119.107 [dataNode.dataNode] [Ariel Writer#events] org.elasticsearch.client.transport.NoNodeAvailableException: No node available
May 8 11:15:28 9.21.119.107 [dataNode.dataNode] [Ariel Writer#events] at org.elasticsearch.client.transport.TransportClientNodesService.execute(TransportClientNodesService.java:219)

[kimchy]

I think that maybe we should validate that trace is actually there when the log4j logger is configured, and fail the node from starting..., this is probably bad on many other places...

[zztv]

This is not real node. It is transport client embedded into our application. TransportClient connects to the real ES nodes. The only place where you can do this kind of checking is constructor of TransportClient by throwing an exception ( which I don't like):

Client client = new TransportClient(settings);

My point is - It is our application fault to use old log4j library, but your diagnostic exception should be clearer. After I replace log4j to newer version everything was fine...

[pickypg]

Related slf4j.org. log4j added trace on August 29, 2005 in 1.2.12. Elasticsearch itself bundles log4j 1.2.17 as an optional dependency.

Any environment that is producing this issue really has a very outdated version of log4j on the classpath (e.g., 1.2.8 was released in early 2003). It certainly is annoying that NoNodeAvailableException occurs because of this, but it's so unexpected that perhaps something else can be done.

What may be useful is to do what @kimchy is getting at, but in a grander way on startup of the client. In essence, create a short integration test that kicks off at startup that tests known potential issues that are deemed worthwhile, and rethrows them as an ElasticsearchIllegalStateException (or similar). Then this could just be a quick one-liner there if it made the cut.

Personally, if we wanted to fail/test for this behavior, then I would actually just add a logger.trace to the top of the function that did something like logger.trace("validating new nodes"); and then change the current trace logger.debug("connecting to node [{}]", node); (or leave as-is, but personally see it as debug anyway). This way, it will fail outside of the try and give a pretty clear Error.

[nikolavp]

We also bumped on this and it isn't cool. Required a remote debugging in a comlpex environment... Can't you just do one of the following:

• Don't log the trace but make it a debug
• Log in the catch Throwable blog with info or warning - this way I will be able to at least see the exception(in this case NoSuchMethodError)

i can understand that this is partly our fault because as @pickypg said, we had an old version of the log4j jar in the classpath.

One more thing would be to throw an IllegalStateException when you do the logger detection in the ESLogger factory if there is no trace level.

[clintongormley]

We should add a check for the ability to log at trace level in the transport client.

[colings86]

We could do this as a very lightweight 'LoggingCheckerModule' started by the transport client which checks it can log at each level

[jasontedor]

As of #16703, we only support log4j on the backend and with #17697 we will support log4j 2 only (which of course, supports trace). Closing.