Skip to content

STORM-904: Move bin/storm command line to java. - #662

Closed
priyank5485 wants to merge 24 commits into
apache:masterfrom
priyank5485:storm-904
Closed

STORM-904: Move bin/storm command line to java.#662
priyank5485 wants to merge 24 commits into
apache:masterfrom
priyank5485:storm-904

Conversation

@priyank5485

Copy link
Copy Markdown
Contributor

No description provided.

@priyank5485

Copy link
Copy Markdown
ContributorAuthor

This PR aims at factoring out the logic for storm command line client which currently resides in two places. One in storm bash script and storm.py for Unix and storm.cmd for Windows. Idea is to create a java program that will be called from storm.cmd and storm bash script which will have all the logic from storm.py and storm.cmd in one place. This will help us maintain the storm client better since we will have only one file to change and in most cases only the base class that both Windows and Unix classes inherit from. I have already removed storm.py. I am working on finishing up the java file and also removing the storm.cmd file. Just putting it out there to elicit response to this approach from the community.

classpath, localconfvalue, remoteconfvalue, repl
activate, deactivate, rebalance, list, help

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If run on Windows ,do nothing but return ?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@caofangkun thanks for checking this out. The code is still not complete. I need to put the windows implementation as well. Most of the code in the UnixStormCommandExecutor will move to abstract base class so that WindowsStormCommandLineExecutor can reuse. It will only have code that is specific to windows.

dev-zookeeper, version, monitor, upload-credentials, get-errors
@knusbaum

Copy link
Copy Markdown
Contributor

I have a little confusion about why we're moving the python to java instead of just having the storm.cmd run the existing storm.py.

I'm all in favor of de-duplicating the storm.py and storm.cmd commands, but I'd rather reuse storm.py than reimplementing it.

@harshach

Copy link
Copy Markdown
Contributor

@knusbaum it will make lot easier for commands to be in java and we can have just wrappers for windows & nix* os. We don't need to have python dependency and in our experience of running python to execute java command to start a jvm takes lot of time and in larger clusters Ambari times out these commands.

@ptgoetz

Copy link
Copy Markdown
Member

Any update on this PR? It would seem it at least needs an upmerge and additional review.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not all process need waitfor(). maybe you can add option. Like this:

public static java.lang.Process launch_process(final String command, final Map<String, String> environment, boolean backend) throws IOException {
if (backend == true) {
new Thread(new Runnable() {
@Override
public void run() {
String[] cmdlist = (new String("nohup " + command + " &")).split(" ");
try {
launchProcess(cmdlist, environment);
} catch (IOException e) {
LOG.error("Failed to run " + command + ":" + e.getCause(), e);
}
}
}).start();
return null;
} else {
String[] cmdlist = command.split(" ");
return launchProcess(cmdlist, environment);
}
}

@longdafeng

Copy link
Copy Markdown
Contributor

@priyank5485

I don't prefer switch storm.py from python to java. Because I often add debug point in the storm.py, it is very easy to debug in online system(especially when environment is bad or jvm parameter is wrong). if we change to java version, it is very hard to debug.

Another point is that python's code is more easy to add new functions in the script.

@priyank5485

Copy link
Copy Markdown
ContributorAuthor

@hustfxj I might have missed something and its been a long time since i did this. I could not find any storm command that would not need a waitFor. Can you give an example? If we do need that flag it should be easy to add and I can change that.

@longdafeng I am not sure I understand the point about debugging. You can still put a break point in this java version of storm command line. I agree with you that its easier to write new functions in script. But the whole point of this JIRA is to have one client that works for Unix based systems and Windows platform. The last time I was working on it we had storm.py for Unix and storm batch client for windows. Both of them also had a separate file for environment variables. It is hard to maintain two scripts anytime we want to change the storm command. We can ask others about their opinion and what they think is better between the two approaches.

@priyank5485

Copy link
Copy Markdown
ContributorAuthor

@longdafeng Also please see one of the previous comments about why not to use python.

d2r pushed a commit to d2r/storm that referenced this pull request Oct 16, 2018
We are closing stale Pull Requests to make the list more manageable.
Please re-open any Pull Request that has been closed in error.
Closesapache#608Closesapache#639Closesapache#640Closesapache#648Closesapache#662Closesapache#668Closesapache#692Closesapache#705Closesapache#724Closesapache#728Closesapache#730Closesapache#753Closesapache#803Closesapache#854Closesapache#922Closesapache#986Closesapache#992Closesapache#1019Closesapache#1040Closesapache#1041Closesapache#1043Closesapache#1046Closesapache#1051Closesapache#1078Closesapache#1146Closesapache#1164Closesapache#1165Closesapache#1178Closesapache#1213Closesapache#1225Closesapache#1258Closesapache#1259Closesapache#1268Closesapache#1272Closesapache#1277Closesapache#1278Closesapache#1288Closesapache#1296Closesapache#1328Closesapache#1342Closesapache#1353Closesapache#1370Closesapache#1376Closesapache#1391Closesapache#1395Closesapache#1399Closesapache#1406Closesapache#1410Closesapache#1422Closesapache#1427Closesapache#1443Closesapache#1462Closesapache#1468Closesapache#1483Closesapache#1506Closesapache#1509Closesapache#1515Closesapache#1520Closesapache#1521Closesapache#1525Closesapache#1527Closesapache#1544Closesapache#1550Closesapache#1566Closesapache#1569Closesapache#1570Closesapache#1575Closesapache#1580Closesapache#1584Closesapache#1591Closesapache#1600Closesapache#1611Closesapache#1613Closesapache#1639Closesapache#1703Closesapache#1711Closesapache#1719Closesapache#1737Closesapache#1760Closesapache#1767Closesapache#1768Closesapache#1785Closesapache#1799Closesapache#1822Closesapache#1824Closesapache#1844Closesapache#1874Closesapache#1918Closesapache#1928Closesapache#1937Closesapache#1942Closesapache#1951Closesapache#1957Closesapache#1963Closesapache#1964Closesapache#1965Closesapache#1967Closesapache#1968Closesapache#1971Closesapache#1985Closesapache#1986Closesapache#1998Closesapache#2031Closesapache#2032Closesapache#2071Closesapache#2076Closesapache#2108Closesapache#2119Closesapache#2128Closesapache#2142Closesapache#2174Closesapache#2206Closesapache#2297Closesapache#2322Closesapache#2332Closesapache#2341Closesapache#2377Closesapache#2414Closesapache#2469
d2r pushed a commit to d2r/storm that referenced this pull request Oct 16, 2018
We are closing stale Pull Requests to make the list more manageable.
Please re-open any Pull Request that has been closed in error.
Closesapache#608Closesapache#639Closesapache#640Closesapache#648Closesapache#662Closesapache#668Closesapache#692Closesapache#705Closesapache#724Closesapache#728Closesapache#730Closesapache#753Closesapache#803Closesapache#854Closesapache#922Closesapache#986Closesapache#992Closesapache#1019Closesapache#1040Closesapache#1041Closesapache#1043Closesapache#1046Closesapache#1051Closesapache#1078Closesapache#1146Closesapache#1164Closesapache#1165Closesapache#1178Closesapache#1213Closesapache#1225Closesapache#1258Closesapache#1259Closesapache#1268Closesapache#1272Closesapache#1277Closesapache#1278Closesapache#1288Closesapache#1296Closesapache#1328Closesapache#1342Closesapache#1353Closesapache#1370Closesapache#1376Closesapache#1391Closesapache#1395Closesapache#1399Closesapache#1406Closesapache#1410Closesapache#1422Closesapache#1427Closesapache#1443Closesapache#1462Closesapache#1468Closesapache#1483Closesapache#1506Closesapache#1509Closesapache#1515Closesapache#1520Closesapache#1521Closesapache#1525Closesapache#1527Closesapache#1544Closesapache#1550Closesapache#1566Closesapache#1569Closesapache#1570Closesapache#1575Closesapache#1580Closesapache#1584Closesapache#1591Closesapache#1600Closesapache#1611Closesapache#1613Closesapache#1639Closesapache#1703Closesapache#1711Closesapache#1719Closesapache#1737Closesapache#1760Closesapache#1767Closesapache#1768Closesapache#1785Closesapache#1799Closesapache#1822Closesapache#1824Closesapache#1844Closesapache#1874Closesapache#1918Closesapache#1928Closesapache#1937Closesapache#1942Closesapache#1951Closesapache#1957Closesapache#1963Closesapache#1964Closesapache#1965Closesapache#1967Closesapache#1968Closesapache#1971Closesapache#1985Closesapache#1986Closesapache#1998Closesapache#2031Closesapache#2032Closesapache#2071Closesapache#2076Closesapache#2108Closesapache#2119Closesapache#2128Closesapache#2142Closesapache#2174Closesapache#2206Closesapache#2297Closesapache#2322Closesapache#2332Closesapache#2341Closesapache#2377Closesapache#2414Closesapache#2469
Sign up for freeto 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.

7 participants

@priyank5485@knusbaum@harshach@ptgoetz@longdafeng@caofangkun@hustfxj