trinidad / trinidad_init_services

Run Trinidad as a service (based on Commons-Daemon and JRuby-JSVC)
Other
28 stars 10 forks source link

init.d/trinidad interprets JAVA_MEM environment variable differently than jruby does #26

Closed tolsen closed 12 years ago

tolsen commented 12 years ago

Hello.

I am starting and stopping init.d/trinidad from inside bundle exec as I have trinidad installed using 'bundle install --deployment'. For example: bundle exec /opt/jux/apps/trinidad/init.d/trinidad start

When I set JAVA_MEM, it is interpreted differently by jruby (as started by bundler) than it is in init.d/trinidad making it impossible to run init.d/trinidad inside a bundle exec:

$ JAVA_MEM=8192m bundle exec /opt/jux/apps/trinidad/init.d/trinidad start
Error: Could not find or load main class 8192m

Further investigation shows that jruby expects JAVA_MEM to be the full option (whereas init.d/trinidad expects only the size):

$ JAVA_MEM=8192m jruby -e 'puts 1'
Error: Could not find or load main class 8192m
$ JAVA_MEM=-Xmx8192m jruby -e 'puts 1'
1
$ 
kares commented 12 years ago

Hey Tim, I'm sorry but this seems a bit weird to me and thus I'm not sure I'd like to advocate such usage ... It's supposed to be a plain-old daemon script on Unix (most likely living under /etc/init.d) bundle exec-ing seems a bit weird (for the Windows part there's even no possibility of doing so), I do understand it might be necessary if Trinidad is part of the bundle but that's actually a detail that might require precious care on a production server.

Seems to me that your use-case might be solved either by using the daemon extension instead (thus eliminating the init.d script since you're using it to manually provide env variables). Or moving bundle exec inside the script or even better making Bundler not care about the trinidad gems when setting up the load path (e.g. by putting it in a separate group) and thus the detail that you're using bundler with your application won't leak up.

Also please note that there are tons of other JVM tunning env variables that you might change on the command line but won't be copied to the Trinidad JVM process Bundler execs ...

tolsen commented 12 years ago

Hi Karol,

My use case may seem weird but the bigger problem is that init.d/trinidad is using an environment variable differently than jruby is. This can cause other problems, for example, if someone places JAVA_MEM in the bashrc of the user account that both runs init.d/trinidad and runs jruby. IMHO, environment variables should not be reused for different purposes like this. I really think this is a bug in init.d/trinidad but I know you just can't change the environment variable name as that will break backward compatibility. That is why I am offering a solution of an alternate environment variable (the old one can be deprecated or not as you wish) as a transition.

Yes, there are other JVM tuning env variables that can be set, but JAVA_MEM is currently the only way to tell init.d/trinidad to set -Xmx. If you don't set JAVA_MEM, init.d/trinidad will use -Xmx500m. One has no choice but to use JAVA_MEM if one wants to change -Xmx.

I'll continue to use my branch for now as I have it working, and it works well for my setup which requires no root access to start or stop the server. I already have trinidad in my Gemfile for development and I prefer to use it out of there for production as the version is more easily kept in sync (via my app's Gemfile.lock) not only across development and my production server, but my staging servers as well.

kares commented 12 years ago

Thanks for getting back to me Tim, I must emphasize that I only pointed out (quite subjectively :)) how I'd approach this "issue" of yours. Quite forgot about the original cause you've described (the JAVA_MEM incompatibility) - that for sure requires a fix, but I think I rather handle that by looking into the value instead of introducing another variable ... and make it's value compatible with the jruby binary. I remember I've previously came across it but left it as it was.

kares commented 12 years ago

084eb3d382f19f5c10138f4d960ace7106326a24 should do it, let me know if you have any issues with the change (see the commit message for details)

tolsen commented 12 years ago

This change works for me! Thanks Karol!