Adding a config for default_server_major_version #170
Conversation
Signed-off-by: Deep Datta <[email protected]>
| if skip_healthcheck == false | ||
| version = get_version(url) | ||
| else | ||
| version = "#{default_server_major_version}.0.0" |
There was a problem hiding this comment.
We're defining default version as 2, should this also be kept as 2 instead of 2.0.0 for consistency?
There was a problem hiding this comment.
The parsing function used to pick out the major version expects the version in this format. Making this 2 was resulting in additional if checks and code clutter.
lib/logstash/outputs/opensearch.rb
Outdated
| config :pipeline, :validate => :string, :default => nil | ||
|
|
||
| # If enabled, skip the GET request to the Healthcheck endpoint to get the server version. | ||
| # The HEAD request is still done to check if the server is alive. |
There was a problem hiding this comment.
What is HEAD request referring to here?
There was a problem hiding this comment.
The healthcheck! function in pool.rb makes 2 request, one via healhealth_check_request() which sends a HEAD to check connectivity. Then a GET via get_version(url) which receives the info json and parses out the version. Some OpenSearch instances respond to the GET with a 404 and empty body, resulting in an exception.
There was a problem hiding this comment.
Should we rename this property? There is somewhat of a health-check with the HEAD request. It isn't quite the same as checking the cluster health, but is somewhat similar. Maybe it should be skip_version_check?
Or an alternative would be to skip it only if default_server_major_version is not defined.
sshivanii
left a comment
sshivanii
left a comment
There was a problem hiding this comment.
Overall this PR looks good to me, only 2 clarifying questions.
|
Similar comment as the templates PR, please include these changes in the README. |
lib/logstash/outputs/opensearch.rb
Outdated
| config :pipeline, :validate => :string, :default => nil | ||
|
|
||
| # If enabled, skip the GET request to the Healthcheck endpoint to get the server version. | ||
| # The HEAD request is still done to check if the server is alive. |
There was a problem hiding this comment.
Should we rename this property? There is somewhat of a health-check with the HEAD request. It isn't quite the same as checking the cluster health, but is somewhat similar. Maybe it should be skip_version_check?
Or an alternative would be to skip it only if default_server_major_version is not defined.
lib/logstash/outputs/opensearch.rb
Outdated
| config :skip_healthcheck, :validate => :boolean, :default => false | ||
|
|
||
| # The OpenSearch server major version to use when it's not available from the Healthcheck endpoint. | ||
| config :default_server_major_version, :validate => :number, :default => 2 |
There was a problem hiding this comment.
If the user skips the healthcheck, maybe it would be good to have them explicitly set the version? Otherwise, they might end up with unexpected behavior. I could go either way on it, but it seems that this advanced feature might require an explicit declaration on their part.
Thoughts? @deepdatta , @sshivanii
There was a problem hiding this comment.
I agree, we can get rid of the skip_heathcheck config.
Put logic in get_version(url) to check if the response body is empty, if so use the default_server_major_version, if its not set then throw an exception. Thoughts @dlvenable, @sshivanii
There was a problem hiding this comment.
@deepdatta , This sounds good. The one change I'd suggestion:
If the get_version(url) throws an exception (perhaps due to 404 or 403), then use default_server_major_version.
Again, if it is not set, we should throw an exception in that case.
Signed-off-by: Deep Datta <[email protected]>
deepdatta
changed the title
3 participants
Signed-off-by: Deep Datta [email protected]
Description
Add a
default_server_major_versionconfig. If this plugin fails to fetch the OpenSearch version number from the host's root URL, it will use this version number, if specified to figure out feature supports.Issues Resolved
Add a config to skip doing a GET on the healthcheck endpoint #165
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.