-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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-2790] Add nimbus admins groups #2390
Conversation
Looks like there is some checkstyle violation issues here. |
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.
Overall the code changes look fine, just some checkstyle violations that need to be addressed and one question about a config that was removed.
@@ -45,6 +47,7 @@ | |||
public class BlobStoreAclHandler { | |||
public static final Logger LOG = LoggerFactory.getLogger(BlobStoreAclHandler.class); | |||
private final IPrincipalToLocal _ptol; | |||
private final IGroupMappingServiceProvider _groupMappingProvider; |
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.
We should move away from using '_' as a prefix to member variables. Please at least fix the new ones to not use it, and if you want to fix all of them that would be great too.
@@ -80,6 +80,7 @@ | |||
} | |||
|
|||
protected Set<String> _admins; | |||
protected Set<String> _adminsGroups; |
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.
Same here we should not use '_' as a prefix.
|
||
public class FixedGroupsMapping implements IGroupMappingServiceProvider { | ||
|
||
public static Logger LOG = LoggerFactory.getLogger(FixedGroupsMapping.class); |
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.
The new guidelines have us use 4 spaces not 2 for indentation.
* implementation to access optional settings. | ||
*/ | ||
@isType(type = Map.class) | ||
public static final String STORM_GROUP_MAPPING_SERVICE_PARAMS = "storm.group.mapping.service.params"; |
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.
Was this just not used anywhere?
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.
It was not used anywhere until this PR - where FixedGroupsMapping
uses it.
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.
+1 pending travis
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.
LGTM. Left some minor comments
@@ -92,6 +93,19 @@ public static void cleanupAfterClass() throws IOException { | |||
// Method which initializes nimbus admin | |||
public static void initializeConfigs() { | |||
conf.put(Config.NIMBUS_ADMINS,"admin"); | |||
conf.put(Config.NIMBUS_ADMINS_GROUPS,"adminsGroup"); | |||
|
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.
nit: space after comma?
@Override | ||
public void prepare(Map storm_conf) { | ||
Map<?, ?> params = (Map<?, ?>) storm_conf.get(Config.STORM_GROUP_MAPPING_SERVICE_PARAMS); | ||
Map<String, Set<String>> mapping = (Map<String, Set<String>>) params.get(STORM_FIXED_GROUP_MAPPING); |
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.
Should we use stormConf
instead of storm_conf
?
Do we want to use Map<String, Object> instead of Map?
…cubator-storm into STORM-2790 STORM-2790: Add nimbus admins groups This closes #2390
No description provided.