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-2038: Disable symlinks with a config option #1974

Merged
merged 2 commits into from
Mar 20, 2017

Conversation

revans2
Copy link
Contributor

@revans2 revans2 commented Feb 27, 2017

This is built on top of #1972 but I have not ported it to other branches yet, so I would like to keep the two separate. Once reviews are done on this version I will port it to 1.x and 1.0.x (as it apparently is a real pain for windows users)

Copy link
Contributor

@HeartSaVioR HeartSaVioR left a comment

Choose a reason for hiding this comment

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

Only one nit for exception message but looks great overall.
Maybe need to test with option off before giving +1.

@@ -341,6 +344,9 @@ public String slurpString(File location) throws IOException {
* @throws IOException on any error.
*/
public void createSymlink(File link, File target) throws IOException {
if (_symlinksDisabled) {
throw new IOException("Symlinks have been disabled, this hsould not be called");
Copy link
Contributor

Choose a reason for hiding this comment

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

hsould -> should

@HeartSaVioR
Copy link
Contributor

Applied the option and submit topology to remote cluster.
No symbolic link created. (find . -type l on storm root doesn't show any.)

+1

@revans2 Great work. Could you resolve conflict? And please create pull requests for 1.x and 1.0.x.

@revans2 revans2 force-pushed the STORM-2038-sym-link-alternative branch from 3a0758b to b000a98 Compare March 16, 2017 17:08
@revans2
Copy link
Contributor Author

revans2 commented Mar 16, 2017

@HeartSaVioR I have rebased the changes to remove the conflict (it was just in the documentation). I'll start working on backporting it.

@ptgoetz
Copy link
Member

ptgoetz commented Mar 16, 2017

+1 @revans2 Was #1972 cherry-picked to the 1.x and 1.0.x branches?

@revans2
Copy link
Contributor Author

revans2 commented Mar 16, 2017

@ptgoetz yes #1972 was a clean cherry pick so I pulled it into 1.x and 1.0.x. This is not so I have #2014 and #2015.

The only real changes for them is that I had to translate the code in Nimbus.java to clojure and modify the many getOrDefault calls to be java 7 compatible.

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