Re-work connections and connection pool in favor of Node Pool #2188

sidz · 2024-01-21T20:51:31Z

This PR re-works Connection Pool, Connections and Strategies in favor of Node Pool from the official client.

As this package is built on top of official client we have to use Node Pool instead of our custom connections mechanism. What makes the whole package a bit simplier to maintain. In the future this could be re-worked again if needed. We would liek to release new version of this package as fast as possible.

ConnectionPool is replaced with Node Pool

More details can be found via link: https://www.elastic.co/guide/en/elasticsearch/client/php-api/8.12/node_pool.html.

By default Official elasticsearch client builds Simple Node Pool with RoundRobin selector and NoResurrect resurrect.

To override default Node Pool behavior do the following:

use Elastic\Transport\NodePool\Resurrect\ElasticsearchResurrect;
use Elastic\Transport\NodePool\Selector\RoundRobin;
use Elastic\Transport\NodePool\SimpleNodePool;

$nodePool = new SimpleNodePool(
    new RoundRobin(),
    new ElasticsearchResurrect()
);

new Client([
    'hosts' => [
        'https://node1.com:9200',
    ],
    'transport_config' => [
        'node_pool' => $nodePool,
    ],
]);

ClientConfiguration changes

ClientConfiguration class is a bit reworked and the following parameters have been removed:

port
path
url
connections in favor of hosts
servers in favor of hosts
roundRobin

host parameter has been renamed to hosts and it's should be an array of strings.

elastic/transport is updated to 8.8 as since that version each host could be configured with its own username/password credentials.

How to configure authentication per node.

new Client([
    'hosts' => [
        'https://username1:password1@node1.com:9200',
        'https://username2:password2@node2.com:9200',
    ],
]);

No longer needed dependecies

symfony/deprecation-contracts
guzzlehttp/psr7
nyholm/dsn

ruflin · 2024-01-22T08:00:12Z

I like the simplification / reduced code base this introduces. One thing that would be useful for the discussion is @sidz is some examples on how users would do it in 7.x and how users would do some of the connection / node pool changes in the future. This should also help guide users that need to migrate to the new behaviour.

sidz · 2024-01-22T09:23:31Z

Yeah, I'll try to add some information as part of this request @ruflin.

All what we called connections now is: hosts and as I can see user is not able to configure multiple connections with different username/password or API Keys in the official client or I didn't find how to do it yet so we are not able to do it either.

pawelkeska · 2024-01-22T10:53:31Z

@sidz @ruflin i left ConnectionPoll from elastica becasue as @sidz writes in elasticsearch-php library we are not able to set multiple connections with diffrent username/password. In the current version on branch 8.x we can do this by set connections like that:

['connections'] => [
    [
       'host' => 'host1',
       'username' => 'example1'
       'password' => 'password1',
       'transport_config' => and here we can set node poll for this connection
    ],
     [
       'host' => 'host2',
       'username' => 'example2'
       'password' => 'password2',
        'transport_config' => and here we can set node poll for this connection
    ],
]

In transport_config key we can set additionally configuration Node Poll for this connection. I think is more flexible for multiple connections with diffrent username/password.

sidz · 2024-01-22T20:16:12Z

Hey @pawelkeska. Thanks for the explanation.

I've looked at what official transport provides and it seems like it is possible to use username and password as part of URI (sense 8.8.0 elastic/elastic-transport-php#22) in the following way:

$builder = TransportBuilder::create();
$build->setHosts([
    'http://user1:password1@node1.com',
    'http://user2:password2@node2.com',
]);

What is made life a bit more easier.

Current solution will work either but our Connection is not a fair single connection/node (or maybe I've missed smth) for a case where amounf of hosts more than one.

If you will open Transport class you will see that when we set an array of hosts to the Transport it will set all of them to the NodePool. So NodePool builds so many Nodes as we pass hosts to the Transport and each Node has it's own URI.

When we call sendRequest on our Client: https://github.com/ruflin/Elastica/blob/8.x/src/Client.php#L644 we will call Transports sendRequest method which works with its NodePool inside of the transport.

And Transport will try with all Nodes defined in the NodePool in the context of our one Connection. So here we have a case when our one Connection works with all Nodes/Hosts.

Ping me if it is not clear. I'll try to give more examples or re-phrase.

pawelkeska · 2024-01-23T09:22:32Z

@sidz Thanks for finding a solution. I haven't seen this PR elastic/elastic-transport-php#22 before and this is good information for us. In this case we could remove our connection pool.

Yes, I agree with you, now is not a fair single connection/node and within one our connection pool we could set many hosts which build so many Nodes as we pass hosts to the Transport and each Node has it's own URI.

If we rebuild this and remove our connection poll is the best solution because we will rely on elastic-transport-php only.

Could you please make corrections in tests and prepare it for merging ?

sidz · 2024-01-23T09:28:33Z

This PR requires a bit more refactoring. I'll try to do it ASAP @pawelkeska.

pawelkeska · 2024-01-23T09:33:05Z

@sidz Thank you for your commitment, it will help us a lot.

ruflin · 2024-01-23T16:47:51Z

Great conversation, not really much to add on my end. My only request is that in the end we have some notes / quicks on how Elastica users can migrate to the new mehtods.

…e official elasticsearch package

…esn't support PSR-17 factories but Elastic\Transport\NodePool\Node rely on psr17 factory

sidz · 2024-01-27T19:36:42Z

composer.json

        "elasticsearch/elasticsearch": "^8.4.1",
-        "guzzlehttp/psr7": "^2.0",
-        "nyholm/dsn": "^2.0.0",


No longer needed as we rely on elastic/transport.

sidz · 2024-01-27T19:36:53Z

composer.json

        "elasticsearch/elasticsearch": "^8.4.1",
-        "guzzlehttp/psr7": "^2.0",


no longer used directly

sidz · 2024-01-27T19:38:18Z

composer.json

@@ -17,12 +17,9 @@
    "require": {
        "php": "~8.0.0 || ~8.1.0 || ~8.2.0",
        "ext-json": "*",
-        "elastic/transport": "^8.4",
+        "elastic/transport": "^8.8",


Since 8.8 elastic/transport supports basic authentication per node. see elastic/elastic-transport-php#22

I assume even if we use 8.8, Elastica will still be compatible with older versions of Elasticsearch 8.x

sidz · 2024-01-27T19:40:57Z

composer.json

@@ -31,9 +28,11 @@
        "phpunit/phpunit": "^9.5",
        "symfony/phpunit-bridge": "^6.0"
    },
+    "conflict": {
+        "guzzlehttp/psr7": "<2.0.0"


We don't have direct dependency to guzzlehttp/psr7 but without this hack --prefer-lowest installs 1.4.0 which doesn't have psr/http-factory and official transport is not able to establish connection (see https://github.com/elastic/elastic-transport-php/blob/main/src/NodePool/Node.php#L33)

I wonder if there is a way we could add a note around this to the code somewhere. I'm worried someone comes along eventually and removes it and we don't see the side effects immediately. The good news is, that now if someone goes back to where it was introduced, they see these comments.

sidz · 2024-01-27T19:42:35Z

tests/Transport/NodePool/TraceableSimpleNodePool.php

+use Elastic\Transport\NodePool\Node;
+use Elastic\Transport\NodePool\SimpleNodePool;
+
+final class TraceableSimpleNodePool extends SimpleNodePool


For testing purposes as original node pool doesn't have getNodes method so we couldn't check different cases in our tests

sidz · 2024-01-27T20:12:08Z

Ready for review @ruflin, @pawelkeska.

While I worked on this PR I started to think that probably we need our own ClientBuilder as a replacement for ClientConfiguration.

ruflin

Overall change LGTM, it simplifies the code.

My main concern is around users migration from 7.x to 8.x. This PR touches the configs that likely every users uses which are the client configs. As you describe, port, path, connections etc. go away / are replaced. To simplify migration, could we keep some of these configs but convert these automatically for users? If services or connectiosn exists, we convert it to hosts. Same if a a port exist, we add it all hosts but log a deprecation warning and let the user know, they should adjust to hosts? This change could also be done as a follow up.

ruflin · 2024-01-22T07:58:30Z

tests/ClientTest.php

@@ -104,25 +104,6 @@ public function testConfigValue(): void
        $this->assertIsArray($client->getConfigValue(['level1', 'level2']));
    }

-    public function testAddHeader(): void


Should we keep this test but adjust it to how users will add headers in the future?

ruflin · 2024-01-31T10:04:57Z

composer.json

@@ -17,12 +17,9 @@
    "require": {
        "php": "~8.0.0 || ~8.1.0 || ~8.2.0",
        "ext-json": "*",
-        "elastic/transport": "^8.4",
+        "elastic/transport": "^8.8",


I assume even if we use 8.8, Elastica will still be compatible with older versions of Elasticsearch 8.x

ruflin · 2024-01-31T10:07:02Z

src/Bulk.php

-        if (!$document->hasRetryOnConflict() && $this->_client->hasConnection() && $this->_client->getConnection()->hasParam('retryOnConflict') && ($retry = $this->_client->getConnection()->getParam('retryOnConflict')) > 0) {
-            $document->setRetryOnConflict($retry);
+        if (!$document->hasRetryOnConflict()) {
+            $retry = $this->_client->getConfigValue('retryOnConflict', 0);


Is this directly related to the change? Maybe make it a quick second PR if not?

Yes. because Client class doesn't have getConnection method so I had to find another way to get configuration value

sidz · 2024-02-10T12:38:39Z

To simplify migration, could we keep some of these configs but convert these automatically for users? If services or connectiosn exists, we convert it to hosts. Same if a a port exist, we add it all hosts but log a deprecation warning and let the user know, they should adjust to hosts?

IMO we need to amend previous version an add deprecation message in the previous (7.x) version instead of in the newest 8. Does it make sense @ruflin ?

ruflin · 2024-02-12T14:37:57Z

IMO we need to amend previous version an add deprecation message in the previous (7.x) version instead of in the newest 8.

In theory I agree. But I think the changes fall into 2 categories:

Changes that can be done in 7.x: We can add a deprecation message and users can already migrate to the new behaviour
Changes that users can't apply in 7.x yet: To make it available in 7.x, we would have to make significant changes to 7.x

I don't have a good understanding on which changes fall where. What can we easily add deprecation in 7.x and make migrate users there, lets do it there. For the ones we can't, we force users to make the changes on upgrade or for the low hanging fruits, lets make changes in 8.x. We can also wait with changes in 8.x and see what stands out for users during the migration and make it easier over time. But especially for configs which 100% of the Elastica users likely have, I think we need to do something.

sidz · 2024-02-18T10:23:53Z

But especially for configs which 100% of the Elastica users likely have, I think we need to do something.

Migration guide should work fine I think.

ruflin · 2024-02-19T15:05:48Z

Lets get it in and work from there. We can always add things. @pawelkeska would be great to get a second eye on this. @sidz From your end, ready to get in?

sidz · 2024-02-19T17:28:06Z

@ruflin yes. ready for review

ruflin · 2024-03-04T09:14:13Z

@sidz Did not forget about the review but was out / busy the last two weeks. Try to get to it this week. Would be great to get some additional reviews from others like @pawelkeska @thePanz etc.

ruflin · 2024-03-18T08:11:59Z

composer.json

@@ -31,9 +28,11 @@
        "phpunit/phpunit": "^9.5",
        "symfony/phpunit-bridge": "^6.0"
    },
+    "conflict": {
+        "guzzlehttp/psr7": "<2.0.0"


I wonder if there is a way we could add a note around this to the code somewhere. I'm worried someone comes along eventually and removes it and we don't see the side effects immediately. The good news is, that now if someone goes back to where it was introduced, they see these comments.

ruflin · 2024-03-18T08:36:16Z

tests/Base.php

    }

    protected function _getHost(): string
    {
-        return \getenv('ES_HOST') ?: Connection::DEFAULT_HOST;
+        return \getenv('ES_HOST') ?: 'localhost';


It would be nice to keep some constants but not sure where to move it. Same below.

ruflin · 2024-03-18T08:42:31Z

Hi @sidz , it took me much longer to get to the review then it should have. Sorry about this.

I must confess, merging this PR was harder then I expected (not because of code reasons). I'm excited about the removal of all the connection code as I will simplify the maintenance of this library. Removing connection was always one of the goals why the dependency on the official client was added and finally we are here. The changes that keep bugging me are the ones where it is breaking for most users but the removal of connections requires it. How will users get to 8 in an easy way?

The answer for me is, the faster we get this in, the more feedback we get and the fewer users already jumped into 8.x and have these changes from day 1. I expect there will be more changes coming to 8.x and I'm looking forward to it together with the community to finally fully support 8.x

@sidz Thank you so much for hanging in there and keep pushing this forward. Will merge this PR now and we will keep iterating on top of it.

sidz · 2024-03-18T09:06:13Z

How will users get to 8 in an easy way?

We could add UPGRADE.MD like Symfony does and provide all steps which users should do in order to migrate from 7 to 8

ruflin · 2024-03-18T09:11:56Z

We could add UPGRADE.MD like Symfony does and provide all steps which users should do in order to migrate from 7 to 8

@sidz ++

sidz force-pushed the drop-custom-connection-and-connection-pools branch 3 times, most recently from b5c906a to 60c6502 Compare January 21, 2024 21:13

ruflin mentioned this pull request Jan 22, 2024

Getting Elastic 8.x to GA #2185

Closed

6 tasks

sidz force-pushed the drop-custom-connection-and-connection-pools branch 3 times, most recently from 119af59 to a0e63b3 Compare January 25, 2024 21:09

Drop own Connection and Connection Pool clases. Use Node Pool from th…

00b376d

…e official elasticsearch package

sidz force-pushed the drop-custom-connection-and-connection-pools branch from a0e63b3 to 00b376d Compare January 25, 2024 21:14

sidz added 2 commits January 27, 2024 22:02

Add conflict with guzzlehttp/psr7 package <2.0.0 as oldest version do…

fabcbd1

…esn't support PSR-17 factories but Elastic\Transport\NodePool\Node rely on psr17 factory

Update change log

ff9abe7

sidz commented Jan 27, 2024

View reviewed changes

composer.json

"elasticsearch/elasticsearch": "^8.4.1",

"guzzlehttp/psr7": "^2.0",

Copy link

Contributor Author

sidz Jan 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no longer used directly

sidz commented Jan 27, 2024

View reviewed changes

sidz force-pushed the drop-custom-connection-and-connection-pools branch from ec08c3a to 6b5afdf Compare January 27, 2024 19:56

Add more tests

cdcf049

sidz force-pushed the drop-custom-connection-and-connection-pools branch from 6b5afdf to cdcf049 Compare January 27, 2024 19:59

Remove setLogger method. (Who need to replace logger at runtime?)

41f79c9

sidz marked this pull request as ready for review January 27, 2024 20:11

ruflin reviewed Jan 31, 2024

View reviewed changes

ruflin approved these changes Mar 18, 2024

View reviewed changes

ruflin merged commit b04f280 into ruflin:8.x Mar 18, 2024
14 of 15 checks passed

sidz deleted the drop-custom-connection-and-connection-pools branch March 18, 2024 09:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-work connections and connection pool in favor of Node Pool #2188

Re-work connections and connection pool in favor of Node Pool #2188

sidz commented Jan 21, 2024 •

edited

Loading

ruflin commented Jan 22, 2024

sidz commented Jan 22, 2024

pawelkeska commented Jan 22, 2024

sidz commented Jan 22, 2024 •

edited

Loading

pawelkeska commented Jan 23, 2024

sidz commented Jan 23, 2024

pawelkeska commented Jan 23, 2024 •

edited

Loading

ruflin commented Jan 23, 2024

sidz Jan 27, 2024

sidz Jan 27, 2024

sidz Jan 27, 2024

ruflin Jan 31, 2024

sidz Feb 10, 2024

sidz Jan 27, 2024

ruflin Mar 18, 2024

sidz Jan 27, 2024 •

edited

Loading

sidz commented Jan 27, 2024 •

edited

Loading

ruflin left a comment •

edited

Loading

ruflin Jan 22, 2024

ruflin Jan 31, 2024

ruflin Jan 31, 2024

sidz Feb 10, 2024

sidz commented Feb 10, 2024

ruflin commented Feb 12, 2024

sidz commented Feb 18, 2024

ruflin commented Feb 19, 2024

sidz commented Feb 19, 2024

ruflin commented Mar 4, 2024

ruflin Mar 18, 2024

ruflin Mar 18, 2024

ruflin commented Mar 18, 2024

sidz commented Mar 18, 2024

ruflin commented Mar 18, 2024

		"elasticsearch/elasticsearch": "^8.4.1",
		"guzzlehttp/psr7": "^2.0",

Re-work connections and connection pool in favor of Node Pool #2188

Re-work connections and connection pool in favor of Node Pool #2188

Conversation

sidz commented Jan 21, 2024 • edited Loading

ConnectionPool is replaced with Node Pool

ClientConfiguration changes

No longer needed dependecies

ruflin commented Jan 22, 2024

sidz commented Jan 22, 2024

pawelkeska commented Jan 22, 2024

sidz commented Jan 22, 2024 • edited Loading

pawelkeska commented Jan 23, 2024

sidz commented Jan 23, 2024

pawelkeska commented Jan 23, 2024 • edited Loading

ruflin commented Jan 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidz Jan 27, 2024 • edited Loading

Choose a reason for hiding this comment

sidz commented Jan 27, 2024 • edited Loading

ruflin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidz commented Feb 10, 2024

ruflin commented Feb 12, 2024

sidz commented Feb 18, 2024

ruflin commented Feb 19, 2024

sidz commented Feb 19, 2024

ruflin commented Mar 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented Mar 18, 2024

sidz commented Mar 18, 2024

ruflin commented Mar 18, 2024

sidz commented Jan 21, 2024 •

edited

Loading

sidz commented Jan 22, 2024 •

edited

Loading

pawelkeska commented Jan 23, 2024 •

edited

Loading

sidz Jan 27, 2024 •

edited

Loading

sidz commented Jan 27, 2024 •

edited

Loading

ruflin left a comment •

edited

Loading