Skip to content
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

support full set of storm commands #60

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

clockfly
Copy link
Contributor

Changes: Support more storm commands in storm-yarn shell.

  1. Add commands. storm-yarn submit, storm-yarn kill, storm-yarn list, storm-yarn rebalance. There is header description for the usage.
  2. Use more robust way to get storm home under a zip. (No need storm version anymore)
  3. fix the JAVA home. For AppMaster, use System.getProperty("java.home"). For supervisor, use env JAVA_HOME.
  4. Allow to pass storm conf when building storm commands.
    For example, to support command: storm-yarn list. We need:
    a) download the updated storm-conf from AppMaster to a temporary folder.
    b) Use the storm-conf in the temporary folder to construct command like: storm list -Dstorm.conf.file=temporary_storm_conf_file.

@revans2
Copy link
Collaborator

revans2 commented Dec 23, 2013

It is going to take me a while to go through the code. I have glanced through it and I am curious why we are calling into the storm command for submit, kill, list, and rebalance? If we want to pass through to storm itself, perhaps it would be preferable to just have a pass through command and by default if storm-yarn cannot find the command requested it just does a passthrough, looking for the appid on the command line, similar to how storm looks for the -c commands on the command line.

@clockfly
Copy link
Contributor Author

Hi Bobby,

I think you means something like this:
storm-yarn -appId xxx -c "storm command".

I think both are good approach, yours may be less code. There are several reasons why we choose current approach instead of this.

  1. Since we need to download a storm conf, and need to change the storm java classpath setting to include the new configuration path, there is no way to do this for "storm-yarn -appId xxx -c "storm command".". As we need to construct the underlying java command ourself.
  2. Current approach will start storm command in the same process of submitter instead of starting a shell command, More easy for us to track exceptions and failures. And we can examine the argument for verification there.
  3. storm-yarn provides full help context for user to use. No need to refer to storm document.Easy to use.

@revans2
Copy link
Collaborator

revans2 commented Dec 30, 2013

OK maybe you didn't understand exactly what I wanted. I would like something like

storm-yarn jar ./myCode.jar -c foo=bar -appId xxx

and

storm-yarn kill -appId xxx -w 0 bar

Where storm-yarn would see jar or kill and know that they should be handled by the storm executable. It would then download the config file, and place it in ~/.storm/appid.yaml, afterwards it would run (Through ProcessBuilder not the shell)

storm jar ./myCode.jar -c foo=bar --config appid.yaml

or

storm kill -w 0 bar --config appid.yaml

I believe that this is more maintainable long term. It would also allow for -c arguments to be processed correctly.

Looking at the code for the storm command I can see that it will not work unless we also ensure that a storm.yaml exists in ~/.storm too. Perhaps the route we want to take is to fix the storm command so that it can take a full path to a config file, and it does not have to be on the classpath.

If you still think that your current code is the best I am fine with that so long as we truly mimic storm behavior and

  1. parse -c options in a compatible way as the storm python script does.
  2. include java.library.path from the configs on the command line when running jar
  3. rename submit to jar so it is compatible with the normal storm. (I can be convinced on this, as jar is possibly confusing here, but for consistency it seems preferable)

@clockfly
Copy link
Contributor Author

clockfly commented Jan 2, 2014

I created a bug for storm about the config path, https://issues.apache.org/jira/browse/STORM-188

I know your point now, basically, we want to parse the configs and MainClass, and then pass everything forward to storm, so that it is more easy for us to maintain the code.

@revans2
Copy link
Collaborator

revans2 commented Jan 2, 2014

Are you planning on putting up a patch for STORM-188, shortly? If not I'll be happy to.

@clockfly
Copy link
Contributor Author

clockfly commented Jan 2, 2014

Yes, I already started working on it. It will be a good practice for me:)

On Thu, Jan 2, 2014 at 11:24 PM, Robert (Bobby) Evans <
[email protected]> wrote:

shortly? If not I'll be happy to.

@clockfly
Copy link
Contributor Author

clockfly commented Jan 3, 2014

Patch submited for https://issues.apache.org/jira/browse/STORM-188

@clockfly
Copy link
Contributor Author

clockfly commented Mar 9, 2014

Patch updated.

It will requires STORM to patch STORM-188 to make this works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants