Official client usage #1275

Merged
merged 3 commits into from Mar 21, 2017

Conversation

Projects
None yet
3 participants
@ewgRa
Collaborator

ewgRa commented Mar 17, 2017

There is old story about implement elasticsearch-php as foundation of Elastica #944

Also as some proposals, that stuck in "client-transport-connection" layer problem, that can't be easy solved.

But we can already use official client benefits without waiting solution for transport layer. I suggest make first step not from solving transport layer, but from starting usage Endpoints.

This PR shows how official client can be used right now, for example for Stats endpoint.
Also as in ClientTest example how Search endpoint can be used.

@ruflin

Great timing about this PR as I wanted to look into this issue once again. This is an awesome improvement!!! 💯

I suggest we split this PR into two parts:

  • Introduce the requestEndpoint method
  • Switch of existing stats to using the official client endpoints

The first one is a no brainer for me. For the second one I trying to understand what the long term benefit will be as it makes a few things more complicated. Don't get me wrong that I think we should not do the second part of the change, just trying to understand more about it.

lib/Elastica/Client.php
@@ -679,6 +680,16 @@ public function request($path, $method = Request::GET, $data = [], array $query
return $response;
}
+ public function requestEndpoint(AbstractEndpoint $endpoint)

This comment has been minimized.

@ruflin

ruflin Mar 20, 2017

Owner

Could you add a comment to this method that it takes each endpoint from elasticsearch-php?

@ruflin

ruflin Mar 20, 2017

Owner

Could you add a comment to this method that it takes each endpoint from elasticsearch-php?

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Mar 20, 2017

Owner

@ewgRa Could you also update the CHANGELOG?

Owner

ruflin commented Mar 20, 2017

@ewgRa Could you also update the CHANGELOG?

@ewgRa

This comment has been minimized.

Show comment
Hide comment
@ewgRa

ewgRa Mar 20, 2017

Collaborator

@ruflin will do requested changes and will split PR later at evening

what the long term benefit

Long term benefit is that it will be easy to follow elasticsearch changes. For example for Stats endpoint. With usage endpoint from official client Elastica doesn't know anything about is this GET or POST request, and what is a "path" for this endpoint. This mean that if something will be changed there, it is just enough to update in composer official client requirement version and it will be work. This information already in official client, why not to use it and reduce maintenance costs for Elastica codebase?

Same for another endpoints. Official client Endpoints have validation of params "checkUserParams" and so on, that also gives advantages, as for Elastica developers, as for end-user. This will make life easier to follow elasticsearch changes. Update version, and check what is broken.

And which one thing it makes more complicated?

Collaborator

ewgRa commented Mar 20, 2017

@ruflin will do requested changes and will split PR later at evening

what the long term benefit

Long term benefit is that it will be easy to follow elasticsearch changes. For example for Stats endpoint. With usage endpoint from official client Elastica doesn't know anything about is this GET or POST request, and what is a "path" for this endpoint. This mean that if something will be changed there, it is just enough to update in composer official client requirement version and it will be work. This information already in official client, why not to use it and reduce maintenance costs for Elastica codebase?

Same for another endpoints. Official client Endpoints have validation of params "checkUserParams" and so on, that also gives advantages, as for Elastica developers, as for end-user. This will make life easier to follow elasticsearch changes. Update version, and check what is broken.

And which one thing it makes more complicated?

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Mar 20, 2017

Owner

Fully agree on your points. The one I was referring to was this one here: https://github.com/ruflin/Elastica/pull/1275/files#diff-b94c222b7bfe828e265627016909c3adR105 The reason is that it requires to add the index which happened before "magically" in the request. I'm thinking if we should add requestEndpoint method in all the places where we have a request method to do the same magic (not part of this PR).

An other good thing about using endpoints as you started to do in this PR is I think in the long run, we will not need our own request method anymore and will fully depend on the base client request layer.

Owner

ruflin commented Mar 20, 2017

Fully agree on your points. The one I was referring to was this one here: https://github.com/ruflin/Elastica/pull/1275/files#diff-b94c222b7bfe828e265627016909c3adR105 The reason is that it requires to add the index which happened before "magically" in the request. I'm thinking if we should add requestEndpoint method in all the places where we have a request method to do the same magic (not part of this PR).

An other good thing about using endpoints as you started to do in this PR is I think in the long run, we will not need our own request method anymore and will fully depend on the base client request layer.

@ewgRa

This comment has been minimized.

Show comment
Hide comment
@ewgRa

ewgRa Mar 20, 2017

Collaborator

@ruflin I get your point about add requestEndpoint to Type and Index, when I prepare PR, I was thinking on it too. The problem there, that in this case we will change Endpoint object inside Elastica code, that is kindly unexpected by developer who use Elastica. Another solution clone Endpoint object and then "setIndex", this looks much better, and I think it is cheap. I'm on position, that in all places where now we have "request" method, must be requestEndpoint method.

yes, later Client::request can be even deprecated, if we will see that it is not needed anymore. For now it is too early for me to say that it is needed. Maybe good idea to not deprecate it, since some users will have cases that require really "raw" queries and don't want make custom Endpoint.

Collaborator

ewgRa commented Mar 20, 2017

@ruflin I get your point about add requestEndpoint to Type and Index, when I prepare PR, I was thinking on it too. The problem there, that in this case we will change Endpoint object inside Elastica code, that is kindly unexpected by developer who use Elastica. Another solution clone Endpoint object and then "setIndex", this looks much better, and I think it is cheap. I'm on position, that in all places where now we have "request" method, must be requestEndpoint method.

yes, later Client::request can be even deprecated, if we will see that it is not needed anymore. For now it is too early for me to say that it is needed. Maybe good idea to not deprecate it, since some users will have cases that require really "raw" queries and don't want make custom Endpoint.

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Mar 20, 2017

Owner

@ewgRa +1 on everything you stated above. I also don't understand yet the long term implication on what we can do with all that in the long term, I only know it allows a lot of great things to happen.

Owner

ruflin commented Mar 20, 2017

@ewgRa +1 on everything you stated above. I also don't understand yet the long term implication on what we can do with all that in the long term, I only know it allows a lot of great things to happen.

@ewgRa ewgRa referenced this pull request in elastic/elasticsearch-php Mar 20, 2017

Merged

endpoint index and type getters #557

@ewgRa

This comment has been minimized.

Show comment
Hide comment
@ewgRa

ewgRa Mar 20, 2017

Collaborator

Ok, I drop changes in Stats and Status and left only changes for request.

What I want to add, it is throwing exception:

    public function requestEndpoint(AbstractEndpoint $endpoint)
    {

        if (null !== $endpoint->getIndex() && $this->getName() != $endpoint->getIndex()) {
            throw new WrongArgumentException("Sorry, it is ambiguous request, which one index we need to request?...");
        }

        $cloned = clone $endpoint;
        $cloned->setIndex($this->getName());
        return $this->getClient()->requestEndpoint($cloned);
    }

And same for type.

But for this we need elastic/elasticsearch-php#557

So, what do you think?

Collaborator

ewgRa commented Mar 20, 2017

Ok, I drop changes in Stats and Status and left only changes for request.

What I want to add, it is throwing exception:

    public function requestEndpoint(AbstractEndpoint $endpoint)
    {

        if (null !== $endpoint->getIndex() && $this->getName() != $endpoint->getIndex()) {
            throw new WrongArgumentException("Sorry, it is ambiguous request, which one index we need to request?...");
        }

        $cloned = clone $endpoint;
        $cloned->setIndex($this->getName());
        return $this->getClient()->requestEndpoint($cloned);
    }

And same for type.

But for this we need elastic/elasticsearch-php#557

So, what do you think?

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Mar 21, 2017

Owner

My suggestion is to move forward with what we have currently in the PR as it is already a huge step forward. For the above I'm glad you opened the PR so we can discuss it further.

Ok for you if I merge it as is?

Owner

ruflin commented Mar 21, 2017

My suggestion is to move forward with what we have currently in the PR as it is already a huge step forward. For the above I'm glad you opened the PR so we can discuss it further.

Ok for you if I merge it as is?

@ewgRa

This comment has been minimized.

Show comment
Hide comment
@ewgRa

ewgRa Mar 21, 2017

Collaborator

@ruflin Sure!

Than I will continue to prepare PR that start use Endpoints for Elastica.

And I have one idea for transport layer, later will try to implement it.

Collaborator

ewgRa commented Mar 21, 2017

@ruflin Sure!

Than I will continue to prepare PR that start use Endpoints for Elastica.

And I have one idea for transport layer, later will try to implement it.

@ruflin ruflin merged commit ca1ddff into ruflin:master Mar 21, 2017

3 checks passed

codecov/patch 100% of diff hit (target 84.35%)
Details
codecov/project 84.4% (+0.04%) compared to ec75c5c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Mar 21, 2017

Owner

@ewgRa Merge. Looking forward to the follow up PR's and I'm especially interested to hear more about the transport layer.

Owner

ruflin commented Mar 21, 2017

@ewgRa Merge. Looking forward to the follow up PR's and I'm especially interested to hear more about the transport layer.

@ewgRa ewgRa deleted the ewgRa:official-client branch Mar 21, 2017

@polyfractal

This comment has been minimized.

Show comment
Hide comment

image

mhernik pushed a commit to mhernik/Elastica that referenced this pull request Jul 24, 2017

Official client usage (#1275)
There is old story about implement elasticsearch-php as foundation of Elastica #944

Also as some proposals, that stuck in "client-transport-connection" layer problem, that can't be easy solved.

But we can already use official client benefits without waiting solution for transport layer. I suggest make first step not from solving transport layer, but from starting usage Endpoints.

This PR shows how official client can be used right now, for example for Stats endpoint.
Also as in ClientTest example how Search endpoint can be used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment