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

STORM-2597: Don't parse passed in class paths #2173

Merged
merged 1 commit into from
Jun 29, 2017

Conversation

revans2
Copy link
Contributor

@revans2 revans2 commented Jun 23, 2017

No description provided.

@harshach
Copy link
Contributor

@revans2 this means the users exported STORM_EXT_CLASSPATH must contain a wildcard "*"
This could result in issues with current users who are just passing the dir and not adding a wildcard and if they have multiple jars in that dir because of this change it will break right

@revans2
Copy link
Contributor Author

revans2 commented Jun 26, 2017

@harshach yes this will cause issues with anyone currently setting a classpath to a directory that has jars in it. But it also make it behave like a java classpath currently does everywhere else.

I am not proposing that we backport this to 1.x, but going forward I would like to see it act differently.

@HeartSaVioR
Copy link
Contributor

HeartSaVioR commented Jun 29, 2017

@revans2 @harshach
I've just played with get_wildcard_dir() and found another issues. Please refer below:

>>> import os
>>> def get_wildcard_dir(path):
...     if os.path.isdir(path):
...         ret = [(os.path.join(path, "*"))]
...     elif os.path.exists(path):
...         ret = [path]
...     return ret
...
>>> get_wildcard_dir("~/WorkArea")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 6, in get_wildcard_dir
UnboundLocalError: local variable 'ret' referenced before assignment
>>> get_wildcard_dir("/Users/jlim/WorkArea")
['/Users/jlim/WorkArea/*']
>>> get_wildcard_dir("/Users/jlim/WorkArea/*")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 6, in get_wildcard_dir
UnboundLocalError: local variable 'ret' referenced before assignment

So get_wildcard_dir() just fails when a part of classpath contains not a dir nor a existing file.
(It even seems not handle ~ character.)
IIRC we don't guide the format of STORM_EXT_CLASSPATH and STORM_EXT_CLASSPATH_DAEMON and then what I expect is the format of Java class path pattern.

I'm +1 for the change. This is a bug hence actually need to port back to 1.x version line, but it might break backward compatibility so not having strong opinion about back port.

Maybe need to fix get_wildcard_dir() as well. Which value we want to return when it's not a dir nor an existing file? Same return value as an existing file? [] to effectively get rid of input to the classpath?

@revans2
Copy link
Contributor Author

revans2 commented Jun 29, 2017

Sounds good. I'll merge it in, and see what, if anything I need to do to pull this into older version of storm too.

@asfgit asfgit merged commit ddf11b1 into apache:master Jun 29, 2017
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.

4 participants