[8.19] (backport #18423) Rewrite Env2yaml in java instead of Go #18456
+479
−306
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* 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/Makefile # docker/data/logstash/env2yaml/env2yaml.go # docker/templates/Dockerfile.erb # docker/templates/IronbankDockerfile.erb # rakelib/artifacts.rake # settings.gradle
Contributor
Author
|
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 |
5 tasks
Member
|
Working through the backport. The template file is significantly different than in 9/main 😅 |
Member
|
run exhaustive tests |
Member
|
Manual build testing:
Ironbank artifact: |
Contributor
Author
|
This pull request has not been merged yet. Could you please review and merge it @donoghuc? 🙏 |
Member
|
🤔: |
Member
|
Oops! Yeah good catch @yaauie 😅 that should for sure have been removed. I must have accidentally reverted that in my merge conflict resolution effort. |
Collaborator
💚 Build Succeeded
History
cc @donoghuc |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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).