Skip to content

STORM-2660 The Nimbus storm-local directory is relative to the workin…#2303

Merged
asfgit merged 1 commit intoapache:masterfrom
HeartSaVioR:STORM-2660
Sep 3, 2017
Merged

STORM-2660 The Nimbus storm-local directory is relative to the workin…#2303
asfgit merged 1 commit intoapache:masterfrom
HeartSaVioR:STORM-2660

Conversation

@HeartSaVioR
Copy link
Contributor

…g directory of the shell executing "storm nimbus"

  • Let Nimbus uses same storm-local directory as Supervisor which is known to be valid

@srdo
Copy link
Contributor

srdo commented Aug 31, 2017

Thanks for taking a look at this. The fix looks good, but I think this is an issue elsewhere too. At least this path is making the same mistake

overrideBase = (String) conf.get(Config.STORM_LOCAL_DIR);
.

Looking at uses of STORM_LOCAL_DIR, I'm wondering if these uses are also suspicious:

@HeartSaVioR
Copy link
Contributor Author

Nice finding @srdo . Addressed other places as well.

Copy link
Contributor

@srdo srdo left a comment

Choose a reason for hiding this comment

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

I think it could be nice to add to the Config javadoc for the blobstore and storm-local dirs that relative paths will be interpreted as relative to storm.home, unless that's already mentioned somewhere.

String stormHome = System.getProperty("storm.home");
String blobStoreDir = (String) conf.get(Config.BLOBSTORE_DIR);
if (blobStoreDir == null) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can make this a little nicer by having this return absoluteStormLocalDir. There's less risk of us accidentally forgetting to null check the result if this ever gets used elsewhere.

@srdo
Copy link
Contributor

srdo commented Sep 1, 2017

+1, verified that storm nimbus now puts storm-local in the right directory.

@HeartSaVioR
Copy link
Contributor Author

@srdo Also addressed review comments and squashed commits into one.

@srdo
Copy link
Contributor

srdo commented Sep 2, 2017

+1, thanks :)

…g directory of the shell executing "storm nimbus"

* Let Nimbus uses same storm-local directory as Supervisor which is known to be valid
* also addresses other places as well (review comment from @srdo)
@asfgit asfgit merged commit 96a0866 into apache:master Sep 3, 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.

3 participants