STORM-3521: Ensures unrecognized Storm CLI flags/arguments are passed to main Java class if any #3146
Conversation
bin/storm.py
Outdated
| jvmtype="-client", | ||
| extrajars=[USER_CONF_DIR, STORM_BIN_DIR]) | ||
|
|
||
| def is_not_conflicting_jar_argument(raw_arg): | ||
| # TODO refactor underlying Topology main classes to avoid this ugly hack | ||
| conflicting_flags = ["-hdfsConf"] |
There was a problem hiding this comment.
Why is it specific to -hdfsConf?
There was a problem hiding this comment.
That's the ugly part - the conflict is that the way the HdfsTopology specifies the argument is non-unix compliant. And there's no consistent way to identify all such possible flags so I'm only filtering this one for now and add more later
There was a problem hiding this comment.
Any idea why flags starting with -h will fail, e.g. -htt?
storm jar: error: argument -h/--help: ignored explicit argument 'tt'
There was a problem hiding this comment.
Because -htt is equivalent to -h -tt. The correct way to specify it is --htt. The HdfsTopology acceps -htt (which is non-Unix compliant it should be single dashes for single characters or double dashes for multiple characters)
There was a problem hiding this comment.
I think I finally find a work around: set allow_abbrev=False.
There was a problem hiding this comment.
That doesn't solve the underlying issue - there are single flag arguments that we had previously in the CLI. Setting that will make it backwards incompatible (like the -c)
There was a problem hiding this comment.
I had an offline discussion with @govind-menon and verified this would work.
If allow_abbrev is true, then -htt will be parsed as -h tt. And that's why it failed.
The only problem is allow_abbrev is not available in python2.
| @@ -1401,7 +1401,7 @@ def drpc_client(args): | |||
|
|
|||
| exec_storm_class( | |||
| "org.apache.storm.command.BasicDrpcClient", | |||
| args=remove_common_options(sys.argv[2:]), storm_config_opts=args.storm_config_opts, | |||
| main_class_args=remove_common_options(sys.argv[2:]), storm_config_opts=args.storm_config_opts, | |||
There was a problem hiding this comment.
why do we need to change this
There was a problem hiding this comment.
This is just to make the name more descriptive
| + '/extlib:/path/to/jar.jar:' + self.storm_dir + '/conf:' + self.storm_dir + '/bin:', | ||
| '-Dstorm.jar=/path/to/jar.jar', '-Dstorm.dependency.jars=', '-Dstorm.dependency.artifacts={}', | ||
| 'some.Topology.Class', '-name', 'run-topology', 'randomArgument', '-randomFlag', 'randomFlagValue', | ||
| '-rotateSize', '0.0001', 'dfs.namenode.kerberos.principal.pattern=hdfs/*.EV..COM', '-hdfsConf' |
There was a problem hiding this comment.
Why is this correct?
'dfs.namenode.kerberos.principal.pattern=hdfs/*.EV..COM', '-hdfsConf'? Shouldn't -hdfsConf appear before 'dfs.namenode.kerberos.principal.pattern=hdfs/*.EV..COM'
There was a problem hiding this comment.
Good catch - the filtered flags are re-added at the end. The argument is of the form -hdfsConf=someValue so the order shouldn't matter
govind-menon
force-pushed
the
STORM-3521
branch
from
5d797e1
to
ae5848b
Compare
govind-menon
changed the title
| '--hdfsConf', 'someOtherHdfsConf', 'dfs.namenode.kerberos.principal.pattern=hdfs/*.EV..COM' | ||
| ], self.mock_execvp, mock.call( | ||
| self.java_cmd, [ | ||
| self.java_cmd, '-client','-Ddaemon.name=', '-Dstorm.options=', |
| ], self.mock_execvp, mock.call( | ||
| self.java_cmd, [ | ||
| self.java_cmd, '-client','-Ddaemon.name=', '-Dstorm.options=', | ||
| '-Dstorm.home=' + self.storm_dir + '', '-Dstorm.log.dir=' + self.storm_dir + "/logs", |
There was a problem hiding this comment.
Can we delete + '' in self.storm_dir + ''
govind-menon
force-pushed
the
STORM-3521
branch
from
ae5848b
to
d86ee85
Compare
… with non unix compliant flags
govind-menon
force-pushed
the
STORM-3521
branch
from
d86ee85
to
3c14c1f
Compare
… with non unix compliant flags