Connection ping only works for timeouts, should also catch dns faillure or node down

[damienalexandre]

The ping method in AbstractConnection is too lazy for me, here is the code:

        try {
            $response = $this->performRequest('HEAD', '', null, null, $options);

        } catch (OperationTimeoutException $exception) {
            $this->isAlive = false;
            return false;

        }

We only catch timeouts, but there is a lot of Exceptions that may occurs here:

• CouldNotResolveHostException
• CouldNotConnectToHost
• OperationTimeoutException
• TransportException

If thinks the 4 of them needs to be catched, if a node goes down (issue or server maintenance) I do not want my whole application to fail.

What do you think about it?

The real issue is, with this code:

        $params = array('hosts' => array (
            '127.0.0.1:9200',
            '127.0.0.1:9201',
        ));
        $this->client = new Client($params);

As the second node does not respond at all, the whole client is crashed with a CouldNotConnectToHost exception - we can't use the first node at all.

[polyfractal]

Yes, definitely agree. I'll fix this right now.

We should be able to catch just TransportException, since all the Curl errors (OperationTimeoutException, etc) and generic node-related errors (ServerErrorResponseException, etc) are children of TransportException

I'll run some test and make sure it doesn't do anything funky. Thanks for filing this bug!

[damienalexandre]

Awesome changes, the b4e71ce commit is great too, thx!

[hellovic]

Sorry if I misunderstand the unit test, but if I changed the test as

public function testOneGoodOneBadHostNoException()
    {
        $params = array('hosts' => array (
            '127.0.0.1:9201',
            '127.0.0.1:9200',
        ));
        $client = new Elasticsearch\Client($params);
        $client->exists(array("index" => 'test', 'type' => 'test', 'id' => 'test'));

    }

An exception may (as host is randomly selected, only 127.0.0.1:9201 will cause error) throw

1. Elasticsearch\Tests\ClientTest::testOneGoodOneBadHostNoException
   Elasticsearch\Common\Exceptions\Curl\CouldNotConnectToHost: Failed to connect to 127.0.0.1 port 9201: Connection refused

Two changes I have made to the
Elasticsearch\Tests\ClientTest::testOneGoodOneBadHostNoException

1. change the first host as the one dead
2. add $client->exists()

It seems to me that the ES client can not bypass the dead host and uses the one alive without exception throw.

[polyfractal]

Good catch, thanks for the bug report!

I was bitten by the infamous PHP isset vs null issue. The retries parameter is set to null in the default configurations so that the user can see what options are available. But because the default retry value depends on the number of hosts, it has to be dynamically configured on client initialization.

So in the default config, retries is set to null. But when dynamically adjusting the value, I was only checking for isset(), so retries was not being set correctly.

Code has been updated, and I tweaked your test to force the issue. I'll tag some releases in a few minutes so that this fix becomes generally available. While I was digging around, I also found another retry bug and an optimization. Thanks!

[hellovic]

I see it's working now.

Thanks Zachary for the quick fix.

BTW, I got another question about dead hosts and here is link