-
Notifications
You must be signed in to change notification settings - Fork 307
Changed conditionally addition of type to open in case of _monitoring #900
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
Changed conditionally addition of type to open in case of _monitoring #900
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code change looks fine - I think it could do with a test, and a change to the changelog would help make the change clearer
CHANGELOG.md
Outdated
@@ -1,5 +1,8 @@ | |||
## 10.2.3 | |||
- Fixed 8.x type removal compatibility issue to continue for monitoring endpoint from LogStash's X-Pack[#899](https://github.com/logstash-plugins/logstash-output-elasticsearch/pull/899) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this review comment accurately describes the change? I think the description given in #899 is more apt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, reworded a little bit. I hope it's not too long
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a suggestion for changelog entry
CHANGELOG.md
Outdated
@@ -1,5 +1,8 @@ | |||
## 10.2.3 | |||
- After remove of type with [#892](https://github.com/logstash-plugins/logstash-output-elasticsearch/pull/892) LogStash X-Pack monitoring started to fail with NPE, (see LogStash #11312) the cause is the ES endpoint used to push monitoring data (\_monitoring/) that needed the type field. Extracted the logic to decide if the type must be removed or not in a separate method and extend the plugin in X-Pack redefining the same method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could do something like:
Opened type removal logic for extension. This allows X-Pack Elasticsearch output to continue using types for special case `/_monitoring` bulk endpoint, enabling a fix for LogStash #11312. [#900](https://github.com/logstash-plugins/logstash-output-elasticsearch/pull/900)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it, done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Changed conditionally addition of type to open in case of _monitoring endpoint
Related to LS #11312 and plugin issue #899