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

Ping Elasticsearch async #14875

Merged
merged 15 commits into from
Dec 14, 2023
Merged

Ping Elasticsearch async #14875

merged 15 commits into from
Dec 14, 2023

Conversation

MikeAlhayek
Copy link
Member

@MikeAlhayek MikeAlhayek commented Dec 9, 2023

Fix #14893

In the past we did not call services.AddElasticServices(); if the Ping() request throw an exception. Note, previously it could fail even without exception. The response.IsValid property was not taken into consideration. So the response could succeed but returns like 404 and we still proceed.

Updated

As per recommendation from @sebastienros , we don't need to ping the server at all. Instead, we should add a Elasticsearch to our health check so it is checked on regular bases.

@MikeAlhayek MikeAlhayek requested review from jtkech and removed request for agriffard and kevinchalet December 11, 2023 15:00
@MikeAlhayek MikeAlhayek merged commit f28560f into main Dec 14, 2023
3 checks passed
@MikeAlhayek MikeAlhayek deleted the ma/feature-manager-async branch December 14, 2023 19:53
}

[RequireFeatures("OrchardCore.Deployment")]
public class DeploymentStartup : StartupBase
{
public override void ConfigureServices(IServiceCollection services)
{
if (services.Any(d => d.GetImplementationType() == typeof(ElasticsearchService)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok that's the part that @sebastienros doesn't like. Then, I'm guessing we are removing this everywhere else too?

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.

Dont ping Elasticsearch during the app bootstraping process
5 participants