Skip to content

Conversation

@smarsching
Copy link
Contributor

This PR introduces three changes to the alarm logging service:

  1. As an alternative to specifying es_host and es_port, it is now possible to specify es_urls. In es_urls a scheme can be specified, so it is possible to use HTTPS instead of HTTP. It is also possible to specify multiple URLs belonging to the same Elasticsearch cluster, thus enabling automatic failover when one of the nodes is not available.
  2. There are new configuration options es_auth_header, es_auth_username, and es_auth_password. es_auth_username and es_auth_password can be used to enable HTTP Basic authentication. As an alternative, the es_auth_header option can be used to directly set the contents auf the HTTP Authorization header, thus enabling arbitrary authentication schemes.
  3. A race condition in ElasticClientHelper.getInstance() has been fixed. This race condition could have the effect that contrary to expectation, more than one ElasticClientHelper was created. It could also result in an uninitialized instance of ElasticClientHelper being returned.

The first two changes are very similar to a PR that was recently merged into the Channel Finder service.

This brings two improvements: First, it is now possible to specify a
scheme (use HTTPS instead of HTTP). Second, more than one host can
be specified, which is useful when using a high-availability cluster.
The authentication can either be performed by specifying a custom
“Authorization” header, or by specifying a username and password.
Due to a lack of synchronization, two instances of ElasticClientHelper
could be created or an uninitialized instance could be returned.
@shroffk shroffk requested review from georgweiss and shroffk April 30, 2024 19:41
Copy link
Collaborator

@georgweiss georgweiss left a comment

Choose a reason for hiding this comment

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

Looks good, but I'd like to keep the default values for es_host and es_port in the properties file as I find it useful when running the service locally with local Elasticsearch.

@smarsching
Copy link
Contributor Author

smarsching commented May 2, 2024

@georgweiss Thank you for your feedback.

I set them to the empty string in the properties file, so that it is easier to detect whether both the URL and the hostname / port has been set (and log an appropriate warning). The setting of http://localhost:9200 still is the effective default, when none of the properties are set.

I could change this and instead detect in the code when more than one of these properties has a non-default value, but I think that this would make the code slightly less readable and it does not really provide any benefits for the user. Would you still prefer that solution?

@shroffk
Copy link
Member

shroffk commented May 2, 2024

I could change this and instead detect in the code when more than one of these properties has a non-default value, but I think that this would make the code slightly less readable and it does not really provide any benefits for the user. Would you still prefer that solution?

I think this would be messy and confusing.

A part of me dislikes have multiple properties controlling the same attributes...but backwards compatibility.

@georgweiss can we mark the old host and port as deprecated and remove them in a future release?

@shroffk shroffk requested review from georgweiss and shroffk and removed request for shroffk May 2, 2024 19:12
@shroffk
Copy link
Member

shroffk commented May 6, 2024

@georgweiss sorry to keep pinging you... but can we merge this?

@georgweiss
Copy link
Collaborator

Sure, go ahead and merge

@shroffk shroffk merged commit 48e22b3 into ControlSystemStudio:master May 6, 2024
@GDH-ISIS
Copy link

This update has certainly improved the connection between the alarm logging service and elasticsearch. Many thanks.

@smarsching smarsching deleted the alarm-logging-es-improvements branch February 21, 2025 22:05
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.

4 participants