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

Create Elasticsearch client for observer only if needed #6407

Merged

Conversation

andreaskapfer
Copy link
Contributor

This PR implements the functionality to create a new ES client for the observer only if needed, i.e. if there are changes in the client config.

It fixes #6090

@elasticmachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@botelastic botelastic bot added the triage label Feb 13, 2023
@thbkrkr thbkrkr added the >enhancement Enhancement of existing functionality label Feb 14, 2023
@botelastic botelastic bot removed the triage label Feb 14, 2023
Copy link
Contributor

@naemono naemono left a comment

Choose a reason for hiding this comment

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

Changes look good. Minor comment changes requested.

pkg/controller/elasticsearch/client/client.go Outdated Show resolved Hide resolved
pkg/controller/elasticsearch/client/client.go Outdated Show resolved Hide resolved
pkg/controller/elasticsearch/client/mock.go Outdated Show resolved Hide resolved
defer tracing.Span(&ctx)()
nsName := k8s.ExtractNamespacedName(&cluster)
settings := m.extractObserverSettings(ctx, cluster)

observer, exists := m.getObserver(nsName)

var esClient client.Client
if exists {
esClient = esClientProvider(observer.esClient)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are effectively doing the comparison twice now here and on L88. Not sure if it worth optimising this further Equals is pretty fast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think for readability it is better to carry out the comparison twice.

pkg/controller/elasticsearch/driver/driver.go Outdated Show resolved Hide resolved
@andreaskapfer andreaskapfer requested review from pebrc and naemono March 31, 2023 11:35
Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/controller/elasticsearch/client/client.go Outdated Show resolved Hide resolved
Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>
@thbkrkr thbkrkr added the v2.8.0 label Mar 31, 2023
@thbkrkr
Copy link
Contributor

thbkrkr commented Mar 31, 2023

buildkite test this

@thbkrkr
Copy link
Contributor

thbkrkr commented Mar 31, 2023

@elasticmachine run elasticsearch-ci/docs

@pebrc pebrc merged commit b581a4f into elastic:main Mar 31, 2023
@barkbay barkbay changed the title Create ES client for observer only if needed Create ELasticsearch client for observer only if needed May 10, 2023
@barkbay barkbay changed the title Create ELasticsearch client for observer only if needed Create Elasticsearch client for observer only if needed May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement Enhancement of existing functionality v2.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create an Elasticsearch client for the observer only if needed
5 participants