Skip to content

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

Conversation

andsel
Copy link
Contributor

@andsel andsel commented Nov 19, 2019

Changed conditionally addition of type to open in case of _monitoring endpoint
Related to LS #11312 and plugin issue #899

Copy link
Contributor

@robbavey robbavey left a 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)
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

@robbavey robbavey left a 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.
Copy link
Contributor

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it, done

Copy link
Contributor

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

LGTM

@elasticsearch-bot
Copy link

Andrea Selva merged this into the following branches!

Branch Commits
master 39a4db9, d86d22f, 648ee9c

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.

3 participants