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-2109: Treat Supervisor CPU/MEMORY Configs as Numbers #1699

Merged
merged 2 commits into from
Sep 25, 2016

Conversation

revans2
Copy link
Contributor

@revans2 revans2 commented Sep 21, 2016

No description provided.

@abellina
Copy link
Contributor

+1

@revans2
Copy link
Contributor Author

revans2 commented Sep 22, 2016

The travis failure is unrelated it is yet again a maven issue downloading something that should be there.

@knusbaum @kishorvpatil could you please take a look?

@HeartSaVioR
Copy link
Contributor

+1

ret.put(Config.SUPERVISOR_CPU_CAPACITY, cpu);
Number mem = (Number) (conf.get(Config.SUPERVISOR_MEMORY_CAPACITY_MB));
ret.put(Config.SUPERVISOR_MEMORY_CAPACITY_MB, mem.doubleValue());
Number cpu = (Number) (conf.get(Config.SUPERVISOR_CPU_CAPACITY));
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibility of NPE here. Can we use utils method to do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. Utils.getDouble() with default value would be safer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The NPE should never happen if defaults.yaml is configured correctly, but I will still make the change.

@revans2
Copy link
Contributor Author

revans2 commented Sep 23, 2016

Rebased and addressed the review comments.

@HeartSaVioR
Copy link
Contributor

+1 again.

@asfgit asfgit merged commit bf91e6d into apache:master Sep 25, 2016
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.

5 participants