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

Add Elasticsearch client cache #2875

Closed
wants to merge 5 commits into from
Closed

Conversation

barkbay
Copy link
Contributor

@barkbay barkbay commented Apr 14, 2020

ECK attempts to reduce the number of API calls to Elasticsearch by storing the api call arguments in annotations.
Unfortunately when ECK fails to update the annotation it may lead to some consistencies: see #2788 and #2864

This PR adds an in memory cache to keep track of the last arguments used when calling the ES API for the following operations:

  • Allocation settings
  • Voting config exclusions
  • Zen1 minimum master nodes
  • Remote clusters

still a draft but ready for review re. the overall architecture

@barkbay barkbay added v1.1.0 >bug Something isn't working labels Apr 14, 2020
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.

Implementationwise this looks good to me. But I am having second thoughts as to whether the complexity of caching ES requests is actually worth it. If I see it correctly we could to a GET before PUT/POST pattern for all of these with the exception of the voting exclusions. GET requests are comparatively cheap compared to the cluster state changing PUT/POST requests iirc and we would side step any cache invalidation/sync issues. Basically: GET the currrent allocation filter, compare and update it only if it's not what we expect. Same for mininum_master_nodes.

type CachedClientBuilder interface {

// NewElasticsearchCachedClient returns an Elasticsearch client that relies on a cache for some operations.
// Do not use the resulting client concurrently for a same operation. Concurrent calls of a given operation is not thread-safe.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Do not use the resulting client concurrently for a same operation. Concurrent calls of a given operation is not thread-safe.
// Do not use the resulting client concurrently for the same operation. Concurrent calls of a given operation are not thread-safe.

steps := []struct {
es1Value, es2Value *string // value to set when calling the api
es1ExpectedValue, es2ExpectedValue *string // expected value stored in the fake client
es1ExpectedCalled, es2ExpectedCalled int // number of time an api is called
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
es1ExpectedCalled, es2ExpectedCalled int // number of time an api is called
es1ExpectedCalled, es2ExpectedCalled int // number of times an api is called

@barkbay
Copy link
Contributor Author

barkbay commented Apr 14, 2020

Discussed out of band: we decided to not rely on any form of cache and always perform sync. calls to Elasticsearch API

@barkbay barkbay closed this Apr 14, 2020
@anyasabo anyasabo removed the v1.1.0 label Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants