-
Notifications
You must be signed in to change notification settings - Fork 66
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
Mesos authentication #37
Conversation
|
||
@SuppressWarnings("unchecked") | ||
protected void initialize(Map conf, String localDir) throws Exception { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please kill this new line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do not like the @SuppressWarnings("unchecked") ??
In general I like to suppress warnings that are not going to get fixed
since the warning in that case adds nothing and its annoying from a
completeness stand point.
But I will remove it.
Thanks
On Fri, May 1, 2015 at 5:26 PM, Timothy Chen notifications@github.com
wrote:
In src/storm/mesos/MesosNimbus.java
#37 (comment):} catch (Exception e) { throw new RuntimeException(e); }
}
- @SuppressWarnings("unchecked")
- protected void initialize(Map conf, String localDir) throws Exception {
Please kill this new line
—
Reply to this email directly or view it on GitHub
https://github.com/mesos/storm/pull/37/files#r29534976.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I see I should have added the issues in my answers I was responding to,
I'm use to threads ....but I will fix all issues.
Thanks.
On Tue, May 5, 2015 at 10:52 AM, Paul Read pdread101@gmail.com wrote:
You do not like the @SuppressWarnings("unchecked") ??
In general I like to suppress warnings that are not going to get fixed
since the warning in that case adds nothing and its annoying from a
completeness stand point.But I will remove it.
Thanks
On Fri, May 1, 2015 at 5:26 PM, Timothy Chen notifications@github.com
wrote:In src/storm/mesos/MesosNimbus.java
#37 (comment):} catch (Exception e) { throw new RuntimeException(e); }
}
- @SuppressWarnings("unchecked")
- protected void initialize(Map conf, String localDir) throws Exception {
Please kill this new line
—
Reply to this email directly or view it on GitHub
https://github.com/mesos/storm/pull/37/files#r29534976.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pdread100 : you misunderstood Tim. He meant "newline" rather than "new line". ;-) The annotation is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Thanks for the clarification.
I have made all the changes requested by @tnachen. Is this sufficient? Thanks. |
hey @pdread100, irrespective of the outcome of @tnachen's response, can you please squash your commits into a single commit, and then force push? So that we get a clean single commit from this change. Thanks! If you need guidance on doing that let me know. |
@pdread100 : I just took a gander at your changes -- it seems that you missed @tnachen's request to not add all these getter/setter methods for variables which are only being used internally to the class. I agree with Tim, it's bloating the class unnecessarily. |
@erikdw I think I can squash them myself and remove the getter/setter since this has been sitting for a while. @pdread100 the changes looks good to me, but can you rebase on master? I cannot apply this PR on latest master. |
@tnachen : great, thanks! |
The last two days I have not had access to the net nor can I address this Thanks On Thu, May 14, 2015 at 7:30 PM, Erik Weathers notifications@github.com
|
6f6a5eb
to
1de8018
Compare
Ok. Rebased, squashed commits, removed the "protected" setters/getters (they were not private), should be good to go. |
@@ -20,10 +20,12 @@ | |||
import backtype.storm.Config; | |||
import backtype.storm.scheduler.*; | |||
import backtype.storm.utils.LocalState; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whitespace.
Open a separate PR for the whitespace/formatting changes. |
What username will the storm mesos framework use to run the storm topology ? Default, i think is root. Can we add it as a parameter too ? |
Mesos ACL will determine what the user tasks/topology is run as. See Mesos Autorization On Tue, May 26, 2015 at 1:30 PM, maverick2202 notifications@github.com
|
Mesos ACL defines rules on how to run the task. The user account comes from the framework. In storm mesos framework, it is not set. FrameworkInfo.Builder finfo = FrameworkInfo.newBuilder() Can we also make it as a parameter ? |
I will double check but I believe you are incorrect. I believe mesos/myriad On Wed, May 27, 2015 at 11:00 AM, maverick2202 notifications@github.com
|
The formatting is all totally out-of-whack after this change. The new code was written with 4-space indents, the old code was 2-space indents, and some of the functions run into each other (no newline in-between). I'll do a reformatting PR. |
I think we need a style checker, similar to what mesos/myriad uses. Such On Wed, May 27, 2015 at 2:50 PM, Erik Weathers notifications@github.com
|
I have created an issue to be worked in response to your request, issue On Wed, May 27, 2015 at 11:00 AM, maverick2202 notifications@github.com
|
Style checkers are better than nothing. Barely. They cannot / don't handle a lot of stuff, I personally hate them.
|
Yeah I'm not a fan either, however I have just started working with open I just ran two different style checkers and this project would require Anyway I will push a PR and folks can test it and if its not a good fit for On Thu, May 28, 2015 at 6:42 AM, Erik Weathers notifications@github.com
|
Sounds great, thanks!
|
Coded and tested Mesos authentication to resolve issue #35