-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[9.1] (backport #18423) Rewrite Env2yaml in java instead of Go #18457
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
* WIP: Rewrite Env2yaml in java instead of Go Managing a Go toolchain for persisting ENV vars in logstash container artifacts has become cumbersome. We already manage a java runtime so this commit presents a path forward to use that instead of Go. The Go binary is faster than java (in my testing Go would complete in around less than 200ms while java takes over 300ms). Given the container startup time is on the order of magnitute of seconds this change should be inperceptable to consumers. The benefit from consolidating in Java is worth the slightly lower performance. * Use TreeMap in java to try to replicate lexicographical order * Explicit imports and TreeMap everywher * Go removals and ironbank workflow update * More non-code removals * Update based on codereview Use snakeyaml-engine and some java flags for faster execution * Build env2yaml in stage Build env2yaml in a separate build stage for container artifacts. Include its dependencies and manage separately from logstash. Continue to use the java runtime in the final container to run the program, but manage the classpath separately. Note this did not use gradle for dependency management because installing that as a depdendcy was not worth it compared with downloading a jar directly. * Use gradle to manage snakeyaml-engine dependency Use gradle (and a dedicated gradle base image) for building env2yaml * Refactor to build env2yaml with gradle rather than in docker build Dont rely on compiling at docker build time, rather do it when logstash compilation is done. * Dont try to use snakeyaml from jruby The complexity around trying to copy over the jar shipped with jruby is not worth how easy it is to just manage it with gradle. This helps with keeping env2yaml contained. * Add license for snakeyaml-engine Licence from https://bitbucket.org/snakeyaml/snakeyaml-engine/src/master/LICENSE.txt * Cleanup and bugfix * Stop skipping empty env vars I mistakenly thought I had observed this behavior in the go version. * Remove quotes from interpolated values Even though we set `.setDefaultScalarStyle(ScalarStyle.PLAIN)` snakeyaml-engine ends up quoting `${}` values. This commit removes them as this was not the behavior with the go version. (cherry picked from commit b15c6c5) # Conflicts: # docker/data/logstash/env2yaml/env2yaml.go # docker/templates/Dockerfile.erb # rakelib/artifacts.rake
|
Cherry-pick of b15c6c5 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
🤖 GitHub commentsJust comment with:
|
💚 Build Succeeded
History
cc @donoghuc |
donoghuc
left a comment
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.
Merge conflicts solved. Going to wait until I get 8.19 fully sorted (and see stability in main/9.2 until I merge this.
donoghuc
left a comment
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.
Comfortable with stability and 8.19 backport. Going to merge.
Release notes
[rn:skip]
What does this PR do?
Managing a Go toolchain for persisting ENV vars in logstash container artifacts has become cumbersome. We already manage a java runtime so this commit presents a path forward to use that instead of Go. The Go binary is faster than java (in my testing Go would complete in around less than 200ms while java takes over 300ms). Given the container startup time is on the order of magnitute of seconds this change should be inperceptable to consumers. The benefit from consolidating in Java is worth the slightly lower performance.
Why is it important/What is the impact to the user?
This should not be noticeable, though technically starting logstash in a container will take about 200ms longer.
Checklist
How to test this PR locally
Build container:
Run env2yaml directly or check at startup.
Example:
This is an automatic backport of pull request #18423 done by [Mergify](https://mergify.com).