Skip to content

Conversation

filipecosta90
Copy link
Contributor

@filipecosta90 filipecosta90 commented Mar 4, 2024

This PR includes changes aimed at enhancing the Elastic connection wrapper.

  • It updates environment variables for configuration, improves the Elasticsearch client initialization with new parameters like timeout settings, and revises the handling of secure connections.
  • Additionally, it cleans up the code by removing redundant configurations and streamlining the process for setting up Elastic clients across different modules.

Copy link
Member

@KShivendu KShivendu left a comment

Choose a reason for hiding this comment

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

Thanks again for the PR! However, I'm not sure if we want to bother about auth and API keys because we don't benchmark cloud deployments. Will think about this.

@filipecosta90 filipecosta90 requested a review from KShivendu April 4, 2024 09:55
@filipecosta90
Copy link
Contributor Author

Thanks again for the PR! However, I'm not sure if we want to bother about auth and API keys because we don't benchmark cloud deployments. Will think about this.

@KShivendu I've removed the API key initialization. I've also extended the configurations that can be tuned and added extra checks concerning the index status and client connection. Please revise this current version

@KShivendu
Copy link
Member

Hi @filipecosta90, I've implemented your refactor in a slightly cleaner way #120

Also, I'm not sure if it makes sense to retry forcing ES into using 1 segment 30 times. Is there a simpler way to do this?

@filipecosta90
Copy link
Contributor Author

Also, I'm not sure if it makes sense to retry forcing ES into using 1 segment 30 times. Is there a simpler way to do this?

we can wrap it around a backoff. but lets close this PR, merge your changes and then if needed I can open a 2nd iteration PR with further changes.

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.

2 participants