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.

Sign up for GitHub

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

Jump to bottom

allow multiple endpoints as dsn to avoid downtime #68

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

allow multiple endpoints as dsn to avoid downtime #68

wants to merge 2 commits into from

Conversation

rliebi
Copy link
Contributor

@rliebi rliebi commented Mar 18, 2024

allow multiple endpoints as dsn to avoid downtime if single node fails. (new config hosts)

@rliebi
Copy link
Contributor Author

rliebi commented Mar 18, 2024

@limenet do you know why the pipeline fails in an untouched area?

@rliebi rliebi requested a review from limenet March 18, 2024 15:29
@rliebi rliebi self-assigned this Mar 18, 2024
@rliebi rliebi marked this pull request as draft March 18, 2024 15:43
@rliebi rliebi removed the request for review from limenet March 18, 2024 15:43
@rliebi rliebi force-pushed the fix-allow-multiple-endpoints-to-ensure-cluster-setup branch from 9015bc6 to 0dedaf4 Compare March 18, 2024 15:59
@rliebi rliebi requested a review from limenet March 18, 2024 19:12
@rliebi rliebi marked this pull request as ready for review March 18, 2024 19:12
@rliebi rliebi force-pushed the fix-allow-multiple-endpoints-to-ensure-cluster-setup branch 2 times, most recently from 81c0e30 to 0dedaf4 Compare March 18, 2024 19:32
@limenet
Copy link
Member

limenet commented Mar 19, 2024

@limenet do you know why the pipeline fails in an untouched area?

This appears to be due to an upstream change https://github.com/ruflin/Elastica/pull/2188/files#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4edR76. Since we currently (have to) require ruflin/elastica as 8.x-dev. This will be taken care of in #69.

@rliebi
Copy link
Contributor Author

rliebi commented Mar 19, 2024

thanks @limenet 🙏

@rliebi rliebi added bug Something isn't working enhancement New feature or request labels Mar 21, 2024
@limenet
Copy link
Member

limenet commented Mar 21, 2024

As discussed:

  • update README / example
  • Consider overriding the factory OR ensure the config allows for all possibilities offered by ruflin/elastica
  • Consider a breaking change to replace string dsn to array dsn

@rliebi
Copy link
Contributor Author

rliebi commented Mar 21, 2024

I see Elastica is changing the API for this right now and it is not clear how it will evolve until Elastica 8.0 is in a final state.
Here my proposal for this feature: for now it is fine to override the Factory and put the settings project specific - but we should implement this as soon Elastica 8.0 is finalised as IMHO this is a crucial feature. We could then also talk about injecting a NodePool directly into the factory. @limenet does this align with your plan for this bundle?

limenet
limenet requested changes Mar 26, 2024
View reviewed changes
src/DependencyInjection/Configuration.php
->scalarNode('url')->end()
->end()
->end()
->end()
->scalarNode('dsn')->defaultValue('http://localhost:9200')->info('The DSN to connect to the Elasticsearch cluster.')->end()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this config node

@limenet limenet added this to the v5.0.0 milestone Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@limenet limenet limenet requested changes

Assignees

@rliebi rliebi

Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Milestone
v5.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@rliebi @limenet