STORM-308 Add support for config_value to {supervisor,nimbus,ui,drpc,logviewer} childopts

[ChitturiPadma]

Added fixes for reading childopts from backtype.storm.command.config_value.

[revans2]

You need the JIRA number in the pull request title.

[revans2]

Why JAVA_HEAP_MAX? I can see that it works, but it is not an intuitive place to put them at all. And what happens if someone else sets JAVA_HEAP_MAX. This will stomp on it.

[ChitturiPadma]

Infact, storm python script doesn't use JAVA_HEAP_MAX. The .cmd files use JAVA_HEAP_MAX as childopts value. If required, we could consider new variable for childopts. My concern is....does JAVA_HEAP_MAX required ? If required, how could we run storm daemons with both JAVA_HEAP_MAX and chilopts value specified on the command line..as they both seem to be in same format as .. -Xmx%%%m

[revans2]

My concern isn't between compatibility with the python code, it is that JAVA_HEAP_MAX is set by storm-config.cmd but in this code it is overwritten, at least I assume that is what = does in a windows batch script. Why not append or prepend it to STORM_OPTS?

[ChitturiPadma]

This is my idea in reading childopts value. JAVA_HEAP_MAX set by storm-config.cmd is overwritten only when {nimbus,ui,supervisor,drpc,logviewer} is invoked. In other scenarios, default of JAVA_HEAP_MAX is used. As per your comments, what i understood is to use different env variable for childopts and prepend this to STORM_OPTS and not to modify JAVA_HEAP_MAX. Is it so ?

[revans2]

That would be fine. The main thing for me is when I see something named JAVA_MAX_HEAP, I only expect things associated with the java heap to ever be placed in it. So putting other things in there is really confusing.

[ChitturiPadma]

Made necessary fixes that would read the childopts and java.library.path from storm.yaml file

[revans2]

Looks OK to me, but I am not a batch file expert. I am +1 on it though.

[d2r]

I had trouble when some of the paths had spaces in them. (I had installed java to c:\Program Files\Java\jdk1.7.0_55.)

I am not a windows cmd syntax expert, but can we handle spaces better by double-quoting as needed?

[ChitturiPadma]

Yes, ofcourse we handle the spaces. Where exactly have u faced issue because of spaces in the paths ?

[d2r]

Starting nimbus fails, probably because the path set for JAVA_HOME is not found:

c:\Users\derekd\Desktop\storm-inst\apache-storm-0.9.2-incubating-SNAPSHOT>bin\st
orm.cmd nimbus
The system cannot find the path specified.
Error: JAVA_HOME is incorrectly set.
'-client' is not recognized as an internal or external command,
operable program or batch file.

Current JAVA_HOME has a space:

c:\Users\derekd\Desktop\storm-inst\apache-storm-0.9.2-incubating-SNAPSHOT>echo %
JAVA_HOME%
c:\Program Files\Java\jdk1.7.0_55

JAVA_HOME exists and is the correct path:

c:\Users\derekd\Desktop\storm-inst\apache-storm-0.9.2-incubating-SNAPSHOT>dir "%
JAVA_HOME%"
 Volume in drive C has no label.
 Volume Serial Number is 

 Directory of c:\Program Files\Java\jdk1.7.0_55

04/29/2014  08:43 PM    <DIR>          .
04/29/2014  08:43 PM    <DIR>          ..
04/29/2014  08:42 PM    <DIR>          bin
03/17/2014  09:09 PM             3,409 COPYRIGHT
04/29/2014  08:42 PM    <DIR>          db
04/29/2014  08:42 PM    <DIR>          include
04/29/2014  08:42 PM    <DIR>          jre
04/29/2014  08:42 PM    <DIR>          lib
04/29/2014  08:42 PM                41 LICENSE
04/29/2014  08:42 PM               123 README.html
04/29/2014  08:42 PM               507 release
03/17/2014  09:09 PM        20,738,566 src.zip
04/29/2014  08:42 PM           125,105 THIRDPARTYLICENSEREADME-JAVAFX.txt
04/29/2014  08:42 PM           176,976 THIRDPARTYLICENSEREADME.txt

I suspected it is because of the space, but it could be some other problem.

[ChitturiPadma]

Yes, when i installed Java in a folder whose name is having spaces, came across the same issue. I wish we could replace %JAVA_HOME% with "%JAVA_HOME%" and ideally this should resolve the issue.

[d2r]

OK, thanks for looking at it.

[ChitturiPadma]

Any more comments on this pull request ?

[d2r]

Sorry, I might have misunderstood your comment to say you were still working on it.
This was an existing issue before your changes.

Created STORM-322.

Moved my Java installation to a path having no spaces

• ran nimbus, ui, supervisor
• ran word_count

I am +1

[ChitturiPadma]

I would welcome any more comments on this. If not, we would change the state of the issue to resolved and would close it if it is merged

[revans2]

No I just got distracted by other things, and haven't had the chance to merge this in. Will try to get to is shortly.