Skip to content
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

ruflin/elastica 6.x is not compatible with elasticsearch/elasticsearch-php 6.8.0 #1908

Closed
reedy opened this issue Mar 7, 2021 · 15 comments
Closed

Comments

@reedy
Copy link
Contributor

reedy commented Mar 7, 2021

Downstream task: https://phabricator.wikimedia.org/T276320

6.x of ruflin/elastica as per https://github.com/ruflin/Elastica/blob/6.x/composer.json#L17 supports "elasticsearch/elasticsearch": "^6.0"

However, trying to use 6.x of ruflin/elastica with 6.8 of elasticsearch/elasticsearch causes errors due to https://github.com/ruflin/Elastica/blob/6.x/lib/Elastica/Index.php#L451

[3e8bfb9228c410ca976223cf] [no req]   Error from line 451 of /var/www/html/vendor/ruflin/elastica/lib/Elastica/Index.php: Class 'Elasticsearch\Endpoints\Indices\Aliases\Update' not found
Backtrace:
#0 /var/www/html/extensions/CirrusSearch/includes/Maintenance/Validators/SpecificAliasValidator.php(137): Elastica\Index->addAlias(string, boolean)
#1 /var/www/html/extensions/CirrusSearch/includes/Maintenance/Validators/SpecificAliasValidator.php(79): CirrusSearch\Maintenance\Validators\SpecificAliasValidator->updateFreeIndices(array)
#2 /var/www/html/extensions/CirrusSearch/includes/Maintenance/Validators/IndexAliasValidator.php(98): CirrusSearch\Maintenance\Validators\SpecificAliasValidator->updateIndices(array, array)
#3 /var/www/html/extensions/CirrusSearch/maintenance/UpdateOneSearchIndexConfig.php(458): CirrusSearch\Maintenance\Validators\IndexAliasValidator->validate()
#4 /var/www/html/extensions/CirrusSearch/maintenance/UpdateOneSearchIndexConfig.php(411): CirrusSearch\Maintenance\UpdateOneSearchIndexConfig->validateSpecificAlias()
#5 /var/www/html/extensions/CirrusSearch/maintenance/UpdateOneSearchIndexConfig.php(269): CirrusSearch\Maintenance\UpdateOneSearchIndexConfig->validateAlias()
#6 /var/www/html/extensions/CirrusSearch/maintenance/UpdateSearchIndexConfig.php(61): CirrusSearch\Maintenance\UpdateOneSearchIndexConfig->execute()
#7 /var/www/html/maintenance/doMaintenance.php(107): CirrusSearch\Maintenance\UpdateSearchIndexConfig->execute()
#8 /var/www/html/extensions/CirrusSearch/maintenance/UpdateSearchIndexConfig.php(70): require_once(string)
#9 {main}

If we look at https://github.com/elastic/elasticsearch-php/blob/6.7.x/src/Elasticsearch/Endpoints/Indices/Aliases/Update.php it still exists in 6.7, but in https://github.com/elastic/elasticsearch-php/blob/6.8.x/src/Elasticsearch/Endpoints/Indices/Aliases/Update.php it does not.

@reedy reedy changed the title ruflin/elastica 6.x is not compatible with elasticsearch-php 6.8 ruflin/elastica 6.x is not compatible with elasticsearch/elasticsearch-php 6.8 Mar 7, 2021
@reedy
Copy link
Contributor Author

reedy commented Mar 7, 2021

It looks to have disappeared in elastic/elasticsearch-php#966 in master...

And in 2da5f9337a269113954a84fe8e79ea6843f0e7c3 in the 6.8 branch...

@reedy
Copy link
Contributor Author

reedy commented Mar 7, 2021

I note a (simple) potential fix is just to set a max version constraint for elasticsearch/elasticsearch (ie <= 6.8) in composer.json, update https://github.com/ruflin/Elastica#versions--dependencies in master/6.x, and then possibly tag a 6.x point release too.

I guess it depends what longer term support goals of ES 6.8 are... And how much (other) work might be needed to update things.

@reedy
Copy link
Contributor Author

reedy commented Mar 7, 2021

Looks like it might just be a missing alias

@ruflin
Copy link
Owner

ruflin commented Mar 8, 2021

Is this only related to the Alias part or is there more? Do you know what it was removed? In general I assume Elastica is compatible with 6.8 just the Alias part causes troubles?

Did not find the time to dig into this but would be good to understand why it was removed.

@reedy
Copy link
Contributor Author

reedy commented Mar 8, 2021

Is this only related to the Alias part or is there more? Do you know what it was removed? In general I assume Elastica is compatible with 6.8 just the Alias part causes troubles?

Yeah. From what I can see (and mentioned on elastic/elasticsearch-php@2da5f93) it seems a few aliases went missing, I think that should be the only issue, but I can't swear to it.

Did not find the time to dig into this but would be good to understand why it was removed.

I think it's accidental, based on @ezimuel's comments. I'm not sure how the aliases are generated, so whether it's an issue with whatever created them automatically, or just human error/oversight and they got missed.

I'm guessing there might not be anything to do "here", certainly when upstream fix this and make a release. At most, maybe some documentation saying not to use that version (depending on semver versions/constraints used, most people would just end up upgrading semi-automatically anyway).

wmfgerrit pushed a commit to wikimedia/mediawiki-extensions-Elastica that referenced this issue Mar 8, 2021
elasticsearch/elasticsearch 6.8.0 was released missing
some class aliases, so currently breaks expected semver
compatibility.

Therefore explicitly require elasticsearch/elasticsearch
~6.5.1 and ~6.7.2.

6.5.x supports ES >= 6.0, < 6.6; 6.5.4 is currently used
in WMF prod.
6.7.x supports ES >= 6.6, < 7.0.

This should be reverted when upstream bugs are fixed.

Upstream tasks:
* ruflin/Elastica#1908
* elastic/elasticsearch-php#1112

Bug: T276320
Bug: T276854
Change-Id: If00e4f871dbfa5572563f1eb5f60ded51f79a06b
wmfgerrit pushed a commit to wikimedia/mediawiki-extensions that referenced this issue Mar 8, 2021
* Update Elastica from branch 'REL1_35'
  to c101a4c17fff7e8711b0199cc9f7c342699e1221
  - Explicitly set elasticsearch/elasticsearch dependency
    
    elasticsearch/elasticsearch 6.8.0 was released missing
    some class aliases, so currently breaks expected semver
    compatibility.
    
    Therefore explicitly require elasticsearch/elasticsearch
    ~6.5.1 and ~6.7.2.
    
    6.5.x supports ES >= 6.0, < 6.6; 6.5.4 is currently used
    in WMF prod.
    6.7.x supports ES >= 6.6, < 7.0.
    
    This should be reverted when upstream bugs are fixed.
    
    Upstream tasks:
    * ruflin/Elastica#1908
    * elastic/elasticsearch-php#1112
    
    Bug: T276320
    Bug: T276854
    Change-Id: If00e4f871dbfa5572563f1eb5f60ded51f79a06b
wmfgerrit pushed a commit to wikimedia/mediawiki-extensions-Elastica that referenced this issue Mar 8, 2021
elasticsearch/elasticsearch 6.8.0 was released missing
some class aliases, so currently breaks expected semver
compatibility.

Therefore explicitly require elasticsearch/elasticsearch
~6.5.1 and ~6.7.2.

6.5.x supports ES >= 6.0, < 6.6; 6.5.4 is currently used
in WMF prod.
6.7.x supports ES >= 6.6, < 7.0.

This should be reverted when upstream bugs are fixed.

Upstream tasks:
* ruflin/Elastica#1908
* elastic/elasticsearch-php#1112

Bug: T276320
Bug: T276854
Change-Id: If00e4f871dbfa5572563f1eb5f60ded51f79a06b
wmfgerrit pushed a commit to wikimedia/mediawiki-extensions that referenced this issue Mar 8, 2021
* Update Elastica from branch 'master'
  to f86b3eb3bbf06b12262be75fce23c876bfbb211e
  - Explicitly set elasticsearch/elasticsearch dependency
    
    elasticsearch/elasticsearch 6.8.0 was released missing
    some class aliases, so currently breaks expected semver
    compatibility.
    
    Therefore explicitly require elasticsearch/elasticsearch
    ~6.5.1 and ~6.7.2.
    
    6.5.x supports ES >= 6.0, < 6.6; 6.5.4 is currently used
    in WMF prod.
    6.7.x supports ES >= 6.6, < 7.0.
    
    This should be reverted when upstream bugs are fixed.
    
    Upstream tasks:
    * ruflin/Elastica#1908
    * elastic/elasticsearch-php#1112
    
    Bug: T276320
    Bug: T276854
    Change-Id: If00e4f871dbfa5572563f1eb5f60ded51f79a06b
@ezimuel
Copy link
Collaborator

ezimuel commented Mar 9, 2021

@ruflin I released elasticsearch-php 6.8.0 including the support of PHP 8 and all the XPack endpoints. In order to accomplish this I used the new code generator used in 7.x branch.

elasticsarch-php < 6.8 contained some endpoints that do not exist in Elasticsearch 6. Moreover, the naming were not aligned with the API specification.

To resolve these issues, I used an alias approach with src/autoload.php to prevent BC breaks. Evidently, some alias was missed.

Here a list of issues provided by @reedy (elastic/elasticsearch-php@2da5f93#commitcomment-47975845):

  • src/Elasticsearch/Endpoints/Cluster/Nodes/AbstractNodesEndpoint.php
    This class has been removed since the new code generator did not use it.

  • src/Elasticsearch/Endpoints/CountPercolate.php
    This endpoint does not exist in Elasticsearch 6. It was present in the PHP client <= 6.7 but it was a typo. This happened because the endpoints was manually maintained in the 6.x branch.

  • src/Elasticsearch/Endpoints/Indices/Aliases/Update.php
    This is a missing alias.

  • src/Elasticsearch/Endpoints/Indices/Exists/Types.php
    This is a missing alias. Actually, there is an alias '\Elasticsearch\Endpoints\Indices\Type\Exists'.

  • src/Elasticsearch/Endpoints/Indices/Field/Get.php
    This is a missing alias. The Elaticsearch 6 endpoint is named indices.get_field_mapping so the endpoint in 6.8.0 is src/Elasticsearch/Endpoints/Indices/GetFieldMapping.php.

  • src/Elasticsearch/Endpoints/Indices/Mapping/Delete.php
    This endpoint does not exist in Elasticsearch 6. It was present in the PHP client <= 6.7 but it was a typo. This happened because the endpoints was manually maintained in the 6.x branch.

  • src/Elasticsearch/Endpoints/Indices/Mapping/Put.php
    This endpoint is already in src/autoload.php.

  • src/Elasticsearch/Endpoints/MPercolate.php
    This endpoint does not exist in Elasticsearch 6. It was present in the PHP client <= 6.7 but it was a typo. This happened because the endpoints was manually maintained in the 6.x branch.

  • src/Elasticsearch/Endpoints/Percolate.php
    This endpoint does not exist in Elasticsearch 6. It was present in the PHP client <= 6.7 but it was a typo. This happened because the endpoints was manually maintained in the 6.x branch.

To summarize, the missing aliases are:

  • Elasticsearch\Endpoints\Indices\Aliases\Update
  • Elasticsearch\Endpoints\Indices\Exists\Types
  • Elasticsearch\Endpoints\Indices\Field\Get

I can fix this and release 6.8.1 soon. Let me know if there are other issues that I didn't count. Thanks!

@phroggar
Copy link

phroggar commented Mar 9, 2021

src/Elasticsearch/Namespaces/IndicesNamespace.php

getAliases() is gone, which was unexpected/breaking for us.

It think this is an alias of getAlias() in 7 and was duplicate code in 6.x.

@ruflin
Copy link
Owner

ruflin commented Mar 10, 2021

@reedy I assume on our end we then wait on the new 6.8.1 release?

@reedy
Copy link
Contributor Author

reedy commented Mar 10, 2021

@reedy I assume on our end we then wait on the new 6.8.1 release?

Pretty much. This task probably mostly serves as documentation purposes, and potentially saves duplicate reports.

Possibly the only resolution (or action) to be taken on this side is maybe some documentation (ie maybe a README entry to say not to use 6.8.0).

With the relatively loose semver constraints, people who ended up on 6.8 (accidentally or otherwise) should just need another composer update when 6.8.1 is released to be ok again. I'm not sure if there's much value in trying to prevent 6.8.0 being installed, at least, not with a technical control.

@ezimuel
Copy link
Collaborator

ezimuel commented Mar 15, 2021

I just sent this PR to fix the issue elastic/elasticsearch-php#1114.

@deguif
Copy link
Collaborator

deguif commented Mar 16, 2021

@reedy Another solution is to add a conflict in composer.json for 6.8.0 version
That's done in #1918

@reedy
Copy link
Contributor Author

reedy commented Mar 16, 2021

I forgot about conflicts :)

@deguif
Copy link
Collaborator

deguif commented Mar 17, 2021

Closing as conflict for elasticsearch/elasticsearch 6.8.0 was added in 6.x branch.
A new release should be published very soon.

@deguif deguif closed this as completed Mar 17, 2021
@reedy reedy changed the title ruflin/elastica 6.x is not compatible with elasticsearch/elasticsearch-php 6.8 ruflin/elastica 6.x is not compatible with elasticsearch/elasticsearch-php 6.8.0 Mar 17, 2021
@ezimuel
Copy link
Collaborator

ezimuel commented Mar 22, 2021

I just released elasticsearch-php 6.8.1: https://github.com/elastic/elasticsearch-php/releases/tag/v6.8.1

@deguif
Copy link
Collaborator

deguif commented Mar 22, 2021

Thank you @ezimuel
Let's see if CI will pick it on 6.x branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants