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

[POC] Implented working config with multiple indexes per document class #917

Open
wants to merge 3 commits into
base: 7.0
Choose a base branch
from

Conversation

toooni
Copy link
Contributor

@toooni toooni commented Feb 11, 2020

I've made a proof of concept for a solution where we can have multiple indexes for one document class again (#902). The config can look like this:

ongr_elasticsearch:
    indexes:
        package_de_chf:
            class: App\Document\Package
            alias: package_de_chf
            hosts:
                - '%env(ELASTICSEARCH_ENDPOINT)%'
        package_fr_chf:
            class: App\Document\Package
            alias: package_fr_chf
            hosts:
                - '%env(ELASTICSEARCH_ENDPOINT)%'
        App\Document\PackageIsochrone:
            hosts:
                - '%env(ELASTICSEARCH_ENDPOINT)%'

If a class parameter is provided the indexes children keys can be a self defined string which will be used as the index service name (if package_de_chf is used, the service will be called ongr.es.index.package_de_chf).

I abused the IndexSettings class to create a temporary storage before creating the definition. I am not sure if this is a good solution or if we should just work with a separate class (only difference would be the propertyMetadata).

I've also removed the return values from the setters since those are not used anywhere.

This PR can probably be targeted to the 6.2 branch too.

@toooni toooni changed the title [POC] Implemtend working config with multiple indexes per document class [POC] Implented working config with multiple indexes per document class Feb 11, 2020
@toooni toooni requested a review from saimaz February 11, 2020 10:12
@toooni toooni changed the base branch from master to 7.0 February 11, 2020 10:39
@codecov-io
Copy link

codecov-io commented Feb 11, 2020

Codecov Report

Merging #917 into 7.0 will increase coverage by 0.99%.
The diff coverage is 82.47%.

Impacted file tree graph

@@             Coverage Diff              @@
##                7.0     #917      +/-   ##
============================================
+ Coverage     83.16%   84.15%   +0.99%     
- Complexity      413      425      +12     
============================================
  Files            39       39              
  Lines          1360     1395      +35     
============================================
+ Hits           1131     1174      +43     
+ Misses          229      221       -8
Impacted Files Coverage Δ Complexity Δ
Mapping/IndexSettings.php 78.94% <64.28%> (+43.81%) 15 <8> (+2) ⬆️
DependencyInjection/Compiler/MappingPass.php 88.39% <85.54%> (-3.92%) 26 <7> (+10)
Mapping/DocumentParser.php 84.52% <0%> (-0.6%) 58% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 428d21e...02be3d3. Read the comment docs.

@toooni
Copy link
Contributor Author

toooni commented Feb 21, 2020

@saimaz Can I have an opinion on the proposed changes? I am not even sure this goes far enough. I think the new approach by using the class name as service name is generally flawed and you should rethink this design decision.
Also having the services public doesn't seem to follow best practices. IMO there should still be something like a manager container. The container would contain the lazy index services and returns them by name without the need of a prefix.

@saimaz
Copy link
Member

saimaz commented Feb 21, 2020

The current architecture does not support multiple indexes for the same document. I'm fully aware that the current approach is not seamless how OOP law directs.

The main idea was to simplify as much as possible to work with the document as the index. For me, personally, the current approach is very handy.

Having said that I'm very open for suggestions, PR's or anything else that might help.

@toooni
Copy link
Contributor Author

toooni commented Feb 21, 2020

Thank you for your response. Ok, with the approach I've implemented both will be possible. Can you review it? I'll create separate PR with my idea for the container approach later.
FYI: I've tested the proposed changes in this PR with our application @ weekend4two.com and had no issues (I've used both class names and manual names for the service names).

@saimaz
Copy link
Member

saimaz commented Feb 21, 2020

Sure, I'l take a look

@toooni
Copy link
Contributor Author

toooni commented Apr 1, 2020

@saimaz Any idea when you will have a chance to look at this PR?

@toooni
Copy link
Contributor Author

toooni commented May 23, 2020

Hi @saimaz. I just wanted to ask again if there is a chance to have this PR reviewed? Or if you don't want to allow the same document used in multiple services?

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

Successfully merging this pull request may close these issues.

None yet

3 participants