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

Elasticsearch6 implementation. #21458

Merged
merged 7 commits into from Mar 2, 2019

Conversation

@romainruaud
Copy link
Collaborator

romainruaud commented Feb 26, 2019

Description (*)

Support Of Elasticsearch 6.

As discussed with @maghamed .

What have been done :

  • Removal of the _all field usage which is not supported anymore.
  • Writing a new Suggestions data provider since the suggest endpoint of Elasticsearch is deprecated.
  • Associated Unit Tests
  • And allow to use ~6.1 version of the PHP client in composer.json

Imho it's enough to deal with the EOL of Elasticsearch 5 and support the 6.

However, the module is quite messy actually (contains many classes and namespaces named after the supported Elasticsearch version, like Elasticsearch5, when Elasticsearch is supposed to be a mixing between generic implementation and legacy 2.x).

The same apply for System configuration fields which are named according to the version of the engine, which is bad imho.

Most probably, it will need a huge refactoring on next stable (2.4) :

  • Cleanup the mess and re-organize the module structure.
  • Deprecate the 5.x support
  • Implement ES 7.x support that will be available soon.

Fixed Issues (if relevant)

  1. magento/community-features#141
  2. ...

Manual testing scenarios (*)

  1. Install version 6 of Elasticsearch
  2. Configure the Engine in the back-office (it uses new configuration fields, like it was done for 5.x support. Imho this is not good, but I implemented the same way).
  3. Run a full reindex and navigate through pages in front, process searches, etc ...

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)
@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

magento-engcom-team commented Feb 26, 2019

Hi @romainruaud. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@magento-cicd2

This comment has been minimized.

Copy link
Contributor

magento-cicd2 commented Feb 26, 2019

CLA assistant check
All committers have signed the CLA.

@orlangur

This comment has been minimized.

Copy link
Contributor

orlangur commented Feb 26, 2019

@romainruaud @maghamed current implementation approach does not look as a robust solution to me.

Most probably, it will need a huge refactoring on next stable (2.4) :

It would be pretty hard if not impossible. Much better solution is to implement Elasticsearch 6 support as a new module, possibly disabled by default, old module may be deprecated as a whole when new one is ready.

@romainruaud

This comment has been minimized.

Copy link
Collaborator Author

romainruaud commented Feb 26, 2019

@orlangur I agree with you on some points.

It's the whole Elasticsearch "abstraction" and the Search part of Magento framework (if I understood properly, ES will be the only supported engine now that MySQL fulltext search implementation is facing deprecation too ?) that has to be revamped in my opinion :

  • Search Requests definitions (generic, to be able to query anything)
  • Search Indices definition (generic, to be able to index anything)
  • Search Query building with abstraction of all ES query and aggregation DSL types
  • Properly customizable search relevance
  • and so on

We can (and we'd enjoy to) help on these topics, since we already built everything listed above in our Open Source implementation of Elasticsearch (which is widely known and used by the community, even documented in the Magento2 devdocs) : https://github.com/Smile-SA/elasticsuite

But this is a huge amount of work to be done and to coordinate.

And for now, Elasticsearch 5 is EOL on 03/11 : https://www.elastic.co/support/eol

So the goal of this PR was to provide support of ES 6 to the community as soon as possible.

Let me know how you wanna process on this PR.

In any cases, I'm not sure there should be a new module "dedicated to Elasticsearch 6", or we will face the same mess on a near future when we will begin to write another module "dedicated to Elasticsearch 7" which is inheriting/extending the other one.

Imho there should be only one module that always handle the latest stable version of Elasticsearch, and that remains compatible with the previous "still maintained" versions with a Deprecation mechanism based on plugins. (So for now, module should fit with ES 6, and handle ES 5 specificity with plugins).
Once a new version of ES is out, we flush all the deprecation plugins, update the default implementation, and build new deprecation plugins. (when ES 7 is out, we provide default implementation of Elasticsearch 7, implement ES 6 specifics as deprecation plugins, and get rid of 5.x completely, because it's EOL). And on and on.

Maybe this part should even be inside Framework directory, or managed on a separate lifecycle.

Product (and more) indexing & querying should be in a separate (most agnostic as possible) modules also.

Let me know,

Best regards

@orlangur

This comment has been minimized.

Copy link
Contributor

orlangur commented Feb 26, 2019

@romainruaud https://github.com/magento/magento2/tree/2.3-develop/app/code/Magento/Tinymce3 is a good example of approach I described.

With all-in-one module it's pretty hard to determine which code parts are responsible for which Elasticsearch version and which versions are actually supported in each particular moment of time.

Elasticsearch6 may be a new standalone module containing everything needed for v6 support. Depending on strategy chosen It may contain plugins for older versions support or such code should be placed in Elasticsearch5 module only.

The same way, when Elasticsearch7 module will be introduced it should provide full support when enabled by itself and may be based on some older code (but not inherit or extend it).

It's enough if only one of Elasticsearch5 / Elasticsearch6 / Elasticsearch7 may be enabled at a time.

@romainruaud

This comment has been minimized.

Copy link
Collaborator Author

romainruaud commented Feb 26, 2019

But with such an approach you will end up with several modules actually sharing 95+% of their code, just like a big copy-paste, and will end up having to fix any issue on all of these nearly identical modules.

Why would you treat supported Elasticsearch versions differently than PHP versions (or any other component) ?

Supporting the latest and "old-stable" on a single module (with proper deprecation mechanism) is enough for me, and seems far easier to maintain.

But we are now a bit far from the initial topic.

@maghamed maghamed self-assigned this Feb 26, 2019

@romainruaud

This comment has been minimized.

Copy link
Collaborator Author

romainruaud commented Feb 26, 2019

Discussed with @maghamed :

  • I'll rework this PR and provide it as a separated module (which will contain only ES6 related logic).

  • Later on, we (or more exactly Magento people) will have to decide which support policy to have with ES versions (either support only latest & old-stable, like for PHP or support more versions with as many separated modules as needed).

I let Igor correct me if needed.

Regards

@CajuCLC

This comment has been minimized.

Copy link

CajuCLC commented Feb 26, 2019

@romainruaud I agree with you that it should be only one module that supports what is still being supported.
I understand that having several modules allows end users to use older versions of ES, but that opens those to security issues in the future. And if end users are not updating ES, most likely they are not updating Magento either.

@miguelbalparda

This comment has been minimized.

Copy link
Collaborator

miguelbalparda commented Feb 26, 2019

@CajuCLC that's not always true since software versions are maintained and upgraded by hosting companies and Magento updates are up to the end users.

@CajuCLC

This comment has been minimized.

Copy link

CajuCLC commented Feb 26, 2019

@miguelbalparda That goes back to what's the right way to maintain your app. IMHO the best way is to maintain everything update so there is minimal security risk.
Does Magento want to make everyone happy? Then why not support PHP 5.3 for all versions then?
You can't make everyone happy, but you can make sure your system is up to date in terms of security. Most of hosting companies will also support older versions of software for some time even if it's already EOL just so it gives their customers time to update.
It all comes down to security. Does Magento plan to leave support to software versions that can be exploited in the future or force customers to update to new versions to get best in class security?

@romainruaud romainruaud force-pushed the romainruaud:feature_es6-compat branch 2 times, most recently from 742fa8c to 4d81048 Feb 26, 2019

@romainruaud

This comment has been minimized.

Copy link
Collaborator Author

romainruaud commented Feb 26, 2019

OK @maghamed I think I updated my PR before you drop your comment on the issue :)

It's now built as a separate module. Nothing altered in the old Magento\Elasticsearch module and everything specifically needed for ES6 in a new one.

Just one point : I'm not completely aware about what to do to ensure the new module will be properly created on the Magento2 composer private repo.

Feel free to ask anything you want more.

Regards

@romainruaud romainruaud force-pushed the romainruaud:feature_es6-compat branch 2 times, most recently from 78bd12a to 0ed7ebc Feb 26, 2019

@romainruaud romainruaud force-pushed the romainruaud:feature_es6-compat branch from 0ed7ebc to 1332aaf Feb 27, 2019

@romainruaud romainruaud force-pushed the romainruaud:feature_es6-compat branch from 1332aaf to ba236f9 Feb 27, 2019

@IvanPletnyov

This comment has been minimized.

Copy link
Contributor

IvanPletnyov commented Feb 27, 2019

@romainruaud Hello. I've created simple product with sku = 'Simple product 1' http://prntscr.com/mqukzb
but in advanced search by sku product isn't founded http://prntscr.com/mqultq .
Is it expected behavior ?

@romainruaud

This comment has been minimized.

Copy link
Collaborator Author

romainruaud commented Feb 27, 2019

@IvanPletnyov this use case does not seem to work on other Elasticsearch implementations (2.x or 5.x).

So most probably, it has never worked as expected.

If it's easy to fix I can have a look but unfortunately, the main topic of this PR is not to fix all existing functional bugs on Elasticsearch implementation, but rather to ensure compatibility with current stable version of ES 6.

Regards

@IvanPletnyov

This comment has been minimized.

Copy link
Contributor

IvanPletnyov commented Feb 27, 2019

@romainruaud Okay, thanks for the answer.

@maghamed
Copy link
Contributor

maghamed left a comment

In general, everything looks good.
@romainruaud you did a great job, and that's a great catch regarding ES6 support in Magento 2.3.1 especially taking into account that EOL of Elasticseach 5 is March 11th of 2019!

We will do our best to make this PR a part of Magento 2.3.1 release

* Return true if third party search engine is used
*
* @return bool
* @since 100.1.0

This comment has been minimized.

@maghamed

maghamed Feb 27, 2019

Contributor

probably this @since 100.1.0 is excessive

This comment has been minimized.

@romainruaud

romainruaud Feb 27, 2019

Author Collaborator

ok fixed.

$prefix = null
) {
parent::__construct($scopeConfig, $clientResolver, $engineResolver, $prefix);
$this->engineResolver = $engineResolver ?: ObjectManager::getInstance()->get(EngineResolverInterface::class);

This comment has been minimized.

@maghamed

maghamed Feb 27, 2019

Contributor

taking into account that it's new module - we can just pass all dependencies as required.
No need to mulate backward compatibility

This comment has been minimized.

@romainruaud

romainruaud Feb 27, 2019

Author Collaborator

I just set them as required.

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

magento-engcom-team commented Feb 27, 2019

Hi @maghamed, thank you for the review.
ENGCOM-4389 has been created to process this Pull Request

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

magento-engcom-team commented Feb 27, 2019

@romainruaud thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

@sidolov sidolov moved this from Pending Review to Merge in Progress in Pull Requests Dashboard Feb 28, 2019

@magento-engcom-team magento-engcom-team merged commit 03f3a4a into magento:2.3-develop Mar 2, 2019

1 of 3 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
licence/cla Contributor License Agreement is signed.
Details

@pull-request-dashboard-manager pull-request-dashboard-manager bot moved this from Merge in Progress to Recently Merged in Pull Requests Dashboard Mar 2, 2019

@contribution-survey

This comment has been minimized.

Copy link

contribution-survey bot commented Mar 2, 2019

Hi @romainruaud, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

magento-engcom-team pushed a commit that referenced this pull request Mar 2, 2019

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.2 milestone Mar 2, 2019

@darksummenors

This comment has been minimized.

Copy link

darksummenors commented Mar 8, 2019

So the ES6 will be supported with version 2.3.2? Thanks romainruaud for the great work! 👍

@romainruaud

This comment has been minimized.

Copy link
Collaborator Author

romainruaud commented Mar 8, 2019

@darksummenors I think Magento is working internally to make it reach the 2.3.1 release pipeline but it's out of my scope. Maybe I was too much "just-in-time" for 2.3.1 if they flagged it for the next release.

Probably @maghamed can clarify this point.

Regards

@tylerssn

This comment has been minimized.

Copy link

tylerssn commented Mar 13, 2019

@romainruaud - thanks for working on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.