-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Java Entrypoint #8161
Java Entrypoint #8161
Conversation
should work out of the box with |
The last time we had lots of ruby code (aka jruby-complete) living in a jar, it had really bad effects on Logstash startup time because (at the time) |
import org.jruby.exceptions.RaiseException; | ||
import org.jruby.runtime.builtin.IRubyObject; | ||
|
||
public final class Main implements Runnable { |
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.
Let's call this Logstash
so that when folks do jps
"Logstash" is what shows up instead of "Main"
This is awesome. What I think this needs to move to completion is a wrapper for arbitrary ruby tasks like I'd rather move the whole thing over than just the main logstash entrypoint for environment consistency. |
@andrewvc sure we can do that, already figured out how and played with that a little, will add that here soon. @jordansissel 's point is valid though, we probably shouldn't pack the |
@andrewvc just to summarize what we talked about:
|
@jordansissel hah I learned a new thing :) Apparently the only reason loading |
c7c3195
to
aece1fc
Compare
@andrewvc this version packages and works just fine (but there are a few open questions around the effects this will have on the IT and plugin test infrastructures ... ITs are red because loading Java first breaks the transitive including of jars for the ITs that include core a Let's look into this whenever you have some time next week:) |
399eb57
to
a10ba72
Compare
@original-brownbear I"m open to reviewing this; lemme know when you think it's ready for testing and review :) |
@jordansissel thanks, hopefully today, will ping you then :) |
a10ba72
to
4f9ae50
Compare
d7944b5
to
3524c70
Compare
@jordansissel alright, now it should be good to review :) |
56e78c7
to
f77c914
Compare
Jenkins test this please |
Sorry, for some reason the Kafka IT is broken now (was all green before a few trivial adjustments), will have to look into that and ping once it's green. |
f77c914
to
cacc790
Compare
This is on my todo list for this week to test/review further. |
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.
Did a brief code review to check for user experience or deployment issues.
I'll test this separately.
bin/logstash
Outdated
@@ -58,5 +58,8 @@ if [ "$1" = "-V" ] || [ "$1" = "--version" ]; then | |||
fi | |||
echo "logstash $LOGSTASH_VERSION" | |||
else | |||
ruby_exec "${LOGSTASH_HOME}/lib/bootstrap/environment.rb" "logstash/runner.rb" "$@" | |||
function join_cp { local IFS=":"; echo "$*"; } | |||
jars=(${LOGSTASH_HOME}/logstash-core/lib/jars/*.jar) |
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.
All systems may not have bash. For example, docker containers or other similarly-minimal Linux environments.
Alternatives
Option 1: Only use a single .jar
This means -cp logstash-core/lib/jars/logstash.jar
and any build changes required to achieve it.
Option 2: Compute classpath at build-time, not runtime.
This would have the build process compute the classpath so that we wouldn't need to compute the classpath during bin/logstash
execution.
Option 3: Same method as you, but with bourne shell (not bash)
If you need to build a class path, you can do this without Bash's array features using $@
which has special meaning in bourne shell.
There may be other ways to achieve this, but here is what I came up with without testing:
Example:
sh-4.4$ ls /tmp/*.jar
/tmp/a.jar /tmp/b.jar '/tmp/c c.jar' /tmp/d.jar
sh-4.4$ function classpath() {
> for i in "$@" ; do
> echo -n "${i}:"
> done | sed -e 's/:$//'
> }
sh-4.4$ classpath /tmp/*.jar; echo
/tmp/a.jar:/tmp/b.jar:/tmp/c c.jar:/tmp/d.jar
## Alternative shell implementation
function classpath() {
echo -n "$1"
shift
while [ $# -gt 0 ] ; do
echo -n ":${1}"
shift
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.
@jordansissel I'd go for 3 here.
- Is really hacky (and I think even problematic legally in some cases) since we have to flatten all jars (or do some other hack that deals with nested jars) into one which entails removing the checksums from their
META-INF
paths => not something we want to do imo. - I don't have a good theoretical argument against this, but the build is already quite complicated isn't it? I'd rather not pile onto that :) Plus I was just able to remove the generated files like logstash_core_jars.rb from the production code paths, I'd rather remove these hacks and not just add a new generated piece of code.
=> sh
is def more portable than bash
, let me try your code :) thanks
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.
@jordansissel hmm your solution works like a charm, but unfortunately only on newer sh
. On vanilla OSX with sh 3.2
the -n
is not understood by echo
and the solution will only work with bash
.
But you know what bash
must be fine for us ... we are currently calling jruby/bin/jruby
from our scripts and it's a bash
script too => we're only compatible if bash
is available right now anyways.
=> Use your solution but with bash
to work on systems with older sh
? :)
require "logstash-core/logstash-core.jar" | ||
rescue Exception => e | ||
raise("Error loading logstash-core/logstash-core.jar file, cause: #{e.message}") | ||
unless $LS_JARS_LOADED |
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.
Hmm.. If we have two places (bin/logstash
and this file) where we compute some classpath things, can we move this to a single place or at least a single language (ruby vs shell vs java)?
Maybe have org.logstash.Logstash
have a method compute this dynamically for use in various places (testing, main method, etc) ?
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.
@jordansissel can't do it :( org.logstash.Logstash
can't load without loading the jars beforehand (doing this kind of thing from Java and then dynamically loading the jars would require a massive hack I think). This we really only need for having the bin/rspec
tool still work.
If we are fine with dropping that tool we don't need it, but I'd rather keep it around so I can debug Ruby comfortably from the IDE :)
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.
This we really only need for having the bin/rspec tool still work.
Can you note this in a code comment?
public static void main(final String... args) { | ||
final String lsHome = System.getenv("LS_HOME"); | ||
if (lsHome == null) { | ||
throw new IllegalStateException("LS_HOME environment variable must be set."); |
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.
Can we make this error message more actionable? If a user sees this exception, what should they do to resolve it, and can we include hints for resolving this in the message?
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.
@jordansissel hmm I think the user can't do anything they see this message (other than open an issue here). This being thrown means there is a bug in the bin/logstash
or bin/logstash.bat
. I simply put this check here for our convenience to have a clean failure instead of some other failure with a strange null
in some string.
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.
other than open an issue here
Ok cool. Let's tell them it's probably a bug in the error message.
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.
Let's include an action to tell them to file an issue or otherwise note that this is a bug and not a user configuration problem.
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.
@jordansissel isn't probably a bug implied in this being a RuntimeException
? :)
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.
As a user, I don't personally associate "this is a bug" with RuntimeException. I'd rather be explicit. Saying "must be set" implies to me that the user should take action to set LS_HOME, which likely isn't the case. Otherwise, a user will end up struggling with this action trying to figure out how, and what value, to set LS_HOME.
resolved = resolved.resolve(element); | ||
} | ||
if (!resolved.toFile().exists()) { | ||
throw new IllegalArgumentException(String.format("Missing: %s.", resolved)); |
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.
Can we make this more actionable, or would a user never expect to see this?
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.
@jordansissel jup same here => user should never ever see this except for when there's a bug :)
if (lsHome == null) { | ||
throw new IllegalStateException("LS_HOME environment variable must be set."); | ||
} | ||
final Path home = Paths.get(lsHome).toAbsolutePath(); |
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.
Can we verify some properties about this path before using it: Does the path exist? Does it have things we want in it? etc..
My thought is we can do a quick check here and provide actionable response to the user if the check fails.
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.
@jordansissel same as the above, this will actually throw something like "file not found" if the path is wrong which we will understand, for the user this means there is a bug in the startup script in 100% of cases imo => nothing actionable they can do right?
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 OK with this, no change required here.
|
If I try to work around the above error (can't find jruby) with this patch: diff --git a/bin/logstash.lib.sh b/bin/logstash.lib.sh
index bb9090808..7f01bcd32 100755
--- a/bin/logstash.lib.sh
+++ b/bin/logstash.lib.sh
@@ -120,7 +120,7 @@ setup_vendored_jruby() {
setup() {
setup_java
- setup_vendored_jruby
+ #setup_vendored_jruby
}
ruby_exec() { Then run
Running
I'm not sure how to test this as |
@jordansissel hmm don't waste your time apparently there is an issue here now. It was green a few weeks ago when I pinged, but I only rebased today without rerunning tests. Seems Jenkins isn't going anywhere as well. Probably just some trivial packaging issue. Will fix tomorrow morning :) |
@original-brownbear noted! :) |
8d328dd
to
6d3dfe3
Compare
@jordansissel with your change to the startup script it works fine for me + on Jenkins (failure is unrelated OOM |
@original-brownbear noted, will test again. Thanks! |
Manual test:
It's good to see |
Tests passing locally. |
require "logstash-core/logstash-core.jar" | ||
rescue Exception => e | ||
raise("Error loading logstash-core/logstash-core.jar file, cause: #{e.message}") | ||
unless $LS_JARS_LOADED |
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.
This we really only need for having the bin/rspec tool still work.
Can you note this in a code comment?
logstash-core/logstash-core.gemspec
Outdated
@@ -53,7 +53,7 @@ Gem::Specification.new do |gem| | |||
|
|||
gem.add_runtime_dependency "sinatra", '~> 1.4', '>= 1.4.6' | |||
gem.add_runtime_dependency 'puma', '~> 2.16' | |||
gem.add_runtime_dependency "jruby-openssl", ">= 0.9.20" # >= 0.9.13 Required to support TLSv1.2 | |||
gem.add_runtime_dependency "jruby-openssl", ">= 0.9.21" # >= 0.9.13 Required to support TLSv1.2 |
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.
Intended? Seems unrelated to the PR.
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.
@jordansissel ha good question so many weeks later :D I think I fixed the problem that this solved elsewhere already => let me revert this :)
public static void main(final String... args) { | ||
final String lsHome = System.getenv("LS_HOME"); | ||
if (lsHome == null) { | ||
throw new IllegalStateException("LS_HOME environment variable must be set."); |
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.
Let's include an action to tell them to file an issue or otherwise note that this is a bug and not a user configuration problem.
if (lsHome == null) { | ||
throw new IllegalStateException("LS_HOME environment variable must be set."); | ||
} | ||
final Path home = Paths.get(lsHome).toAbsolutePath(); |
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 OK with this, no change required here.
@@ -1,4 +1,4 @@ | |||
#!/bin/sh | |||
#!/bin/bash |
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.
this should still be /bin/sh
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.
@jordansissel bash
is more portable here imo :( Your code only works with sh
4.x
=> bash
3.x
works fine though => standard OSX and old Linux will break if we go with sh
=> bash
wins, especially since JRuby requires bash
anyways in the way we currently used to invoke it? :)
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.
Hmm, fair. I didn't realize that bin/jruby invoked bash.
* Created `org.logstash.Logstash` as entrypoint * Safely handle `Ruby` runtime (which sadly is still a singleton, moving away from that will require a few iterations on top of this) * Adjusted `bat` and `sh` entry point wrappers * Verified manually that performance is unchanged (i.e. all Java opts are still loaded properly) * Flattened `.jar` path to make it a little less bothersome to build the `-cp` string * Retained ability to load jars from Ruby via the global `$LS_JARS_LOADED` variable hack, to keep plugin specs that load LS as a `.gem` functional (like e.g. the ITs in LS itself) * No need for the gem jars magic anymore, the downloading and moving into place of jars is now all handled by Gradle
6d3dfe3
to
166c88c
Compare
@jordansissel all points addressed I think/hope :) |
Yep! I still need to test on windows. Should be done building shortly :) |
@jordansissel see PR description at least for me Windows worked fine :D *fingers crossed :P |
@original-brownbear 🤦♂️ I missed the screenshot. LGTM |
@jordansissel thanks! :) |
* Created `org.logstash.Logstash` as entrypoint * Safely handle `Ruby` runtime (which sadly is still a singleton, moving away from that will require a few iterations on top of this) * Adjusted `bat` and `sh` entry point wrappers * Verified manually that performance is unchanged (i.e. all Java opts are still loaded properly) * Flattened `.jar` path to make it a little less bothersome to build the `-cp` string * Retained ability to load jars from Ruby via the global `$LS_JARS_LOADED` variable hack, to keep plugin specs that load LS as a `.gem` functional (like e.g. the ITs in LS itself) * No need for the gem jars magic anymore, the downloading and moving into place of jars is now all handled by Gradle Fixes #8161
* Created `org.logstash.Logstash` as entrypoint * Safely handle `Ruby` runtime (which sadly is still a singleton, moving away from that will require a few iterations on top of this) * Adjusted `bat` and `sh` entry point wrappers * Verified manually that performance is unchanged (i.e. all Java opts are still loaded properly) * Flattened `.jar` path to make it a little less bothersome to build the `-cp` string * Retained ability to load jars from Ruby via the global `$LS_JARS_LOADED` variable hack, to keep plugin specs that load LS as a `.gem` functional (like e.g. the ITs in LS itself) * No need for the gem jars magic anymore, the downloading and moving into place of jars is now all handled by Gradle Fixes elastic#8161
Java entry point:
org.logstash.Logstash
as entrypointRuby
runtime (which sadly is still a singleton, moving away from that will require a few iterations on top of this)bat
andsh
entry point wrappers.jar
path to make it a little less bothersome to build the-cp
string$LS_JARS_LOADED
variable hack, to keep plugin specs that load LS as a.gem
functional (like e.g. the ITs in LS itself)Description incoming
Works on Windows: