New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review for integration of official PHP library #989

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@CrAzE124
Contributor

CrAzE124 commented Nov 23, 2015

Added "elasticsearch/elasticsearch" to the composer.json for integration with the official PHP client;
Updated the current Elastica\Client::request to use the transport from the Elasticsearch\Client member variable;
Added a new member to the Elastica\Client class to house an instance of Elasticsearch\Client;
TODO - Do we want to stick with Aggregation over Inheritance?
Added a factory method to get the appropriate type of ConnectionException - WIP;

This is all a WIP PR.

References #944

Added "elasticsearch/elasticsearch" to the composer.json for integrat…
…ion with the official PHP client;

Updated the current Elastica\Client::request to use the transport from the Elasticsearch\Client member variable;
Added a new member to the Elastica\Client class to house an instance of Elasticsearch\Client;
TODO - Do we want to stick with Aggregation over Inheritance?
Added a factory method to get the appropriate type of ConnectionException - WIP;
*/
public static function getConcreteConnectionException(AbstractTransport $transport)
{
return new GuzzleException(new TransferException());

This comment has been minimized.

@CrAzE124

CrAzE124 Nov 23, 2015

Contributor

This is obviously not correct - just a PoC.

@ruflin

This comment has been minimized.

Owner

ruflin commented Nov 23, 2015

@CrAzE124 Very interesting. I wasn't even thinking of what you just did here but this is a very interesting approach. It makes it possible to first test if the integration works as expected and move in a second step.

In the long term my idea is that elasticsearch-php replaces to full transport and connection layer of Elastica. This is the part where the two libraries have a lot of code in common and improvements always have to be made on both sides. So elasticsearch-php is like the transport and connection library of Elastica :-)

@CrAzE124

This comment has been minimized.

Contributor

CrAzE124 commented Nov 23, 2015

@ruflin yes I thought that was your plan! If you're happy with the general idea (and with the use of a couple more adapter classes) I can move forward on this sometime in the week?

As far as I understand the requirement, we need to refactor the codebase to implement the transport and connection facilities as provided by php-elasticsearch, whilst maintaining backwards compatibility. Correct?

@ruflin

This comment has been minimized.

Owner

ruflin commented Nov 24, 2015

The question is how much more work would it be to go the full way and extend Elastica\Client by elasticsearch-php client and remove all Transport and connections on the Elastica side? With moving to elasticsearch 2.0 we will have lots of BC breaks, so it would also be a good timing to do the above. The reason I like to go the full way directly is because it will also remove a large portion of code on our side. The main method that would have to be changed is Elastica\Client::request(...): https://github.com/ruflin/Elastica/blob/master/lib/Elastica/Client.php#L615 Inside it should do what you did in the change above. If this is the case, we could probably even change it without major BC breaks if someone doesn't have very special transport and connection configs.

Currently it sounds quite simple but I'm sure I forgot a lot of things. I'm happy to also jump on a call to have direct conversation which could be worth for the above case.

@CrAzE124

This comment has been minimized.

Contributor

CrAzE124 commented Nov 24, 2015

@ruflin yes I see what you're saying with regards to Transport and Connections. I think it can be done without much BC breakage.

I think we should schedule a Skype/Hangouts session - pop me a mail (I believe my email is public) and we can set something up!

@ruflin

This comment has been minimized.

Owner

ruflin commented Dec 2, 2015

@CrAzE124 Any updates here. One thing that came to my mind which probably is going to be a challenge: Map the configuration options from Elastica\Client to elasticsearch-php transport.

@CrAzE124

This comment has been minimized.

Contributor

CrAzE124 commented Dec 7, 2015

@ruflin sorry, been extremely busy with work! Will be looking at this again tonight.

@ruflin

This comment has been minimized.

Owner

ruflin commented Dec 7, 2015

👍

@CrAzE124

This comment has been minimized.

Contributor

CrAzE124 commented Dec 7, 2015

@ruflin 2 questions,

1 - Are we trying to maintain BC with this? I'm thinking I can just about strip out the entire Connection and Transport namespaces if we don't need to maintain BC. If we do, perhaps I'll just replace the guts of the classes in these namespaces with some nice little wrappers.
2 - What version of Elastica should I be building against?

@ruflin

This comment has been minimized.

Owner

ruflin commented Dec 8, 2015

For BC: If possible, but not required. I'm happy to get rid of the Connection and Transport layer as this would only be "half" a BC break, as I assume most of the people are not accessing it directly. The question here is more about the configuration options if we keep it or not.

For Elastica version: 3.0.*. Based on how fast we get this in we could even get it in for version 3 as we are still in beta, otherwise I would also be willing to go to 3.1 or even 4.0 quite soon.

@ruflin

This comment has been minimized.

Owner

ruflin commented Jan 4, 2016

@CrAzE124 Did you make any progress here?

@ewgRa

This comment has been minimized.

Collaborator

ewgRa commented Feb 1, 2016

ping @CrAzE124 ? Do you need any help here?

@CrAzE124

This comment has been minimized.

Contributor

CrAzE124 commented Feb 1, 2016

@ewgRa sorry, yes, I think I might be in over my head here.

@ruflin

This comment has been minimized.

Owner

ruflin commented May 13, 2017

@ewgRa @CrAzE124 Reviewing old PR's I was stumbling over this one. By now we have the official elasticsearch client for queries. I still really like this idea to also have it as an adapter. As soon as we have it as an adapter and it works well, it could also be enabled as default adapter in the long run. Anyone interested to push this one forward?

@ewgRa

This comment has been minimized.

Collaborator

ewgRa commented May 13, 2017

@ruflin I'm still interested, but want return to it later, have another several things in priority and not so much free time.

But from my point of view, make it as adapter - it is partial solution, no? And it is not enough. The best solution - all transport layer, as low-level layer, must be based on official client. So, all adapters, any cases of connections (AWS, etc.) must be supported by official client and must be out of scope for Elastica.
Of course, this is a broke backward compatibility, but it is forward step, not a side step as adapter.

@ruflin

This comment has been minimized.

Owner

ruflin commented May 15, 2017

@ewgRa Fully agree. The end result will be what you described above. The part I'm hoping with first going with the adapter is that it allows us to learn on how it works and it splits up the work in smaller and more digestable pieces. If you want to go all in, also good for me :-D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment