----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34180/#review84059 -----------------------------------------------------------
security-admin/src/main/java/org/apache/ranger/biz/ServiceMgr.java <https://reviews.apache.org/r/34180/#comment135187> Instead of hardcoding valicateConfig-timeout = lookup-timeout * 10, I think it will be better to use 2 different configurations - one for lookup and another for validate-config. security-admin/src/main/java/org/apache/ranger/common/LookupConfigurator.java <https://reviews.apache.org/r/34180/#comment135190> timeout.unit as a configurable value may not be very useful here. It should be enough just to support milliseconds in "timeout.value". Please consider replacing 2 properties (timeout.value and timeout.unit) with 1 (timeout.value.in.ms). One less property for the users to configure. security-admin/src/main/java/org/apache/ranger/common/LookupConfigurator.java <https://reviews.apache.org/r/34180/#comment135191> resourceLookup and configValidate operations might require different timeout values. It will be good to support 2 different properties. resource.lookup.timeout.in.ms config.validate.timeout.in.ms security-admin/src/main/java/org/apache/ranger/common/LookupConfigurator.java <https://reviews.apache.org/r/34180/#comment135193> Why static to store these values? The value could be different for different services. Also these values would be retrieved by looking into a map - so there is not much to gain by caching the value. security-admin/src/main/java/org/apache/ranger/common/LookupConfigurator.java <https://reviews.apache.org/r/34180/#comment135192> Consider the following order to retrieve config: 1. lookup.timeout.value in config map of the service 2. ranger.service.hadoopdev.lookup.timeout.value in Ranger admin config # service specific config 3. ranger.servicetype.hdfs.lookup.timeout.value # all service instances of a service-type 4. ranger.service.lookup.timeout.value # all service instances 5. lookup.timeout.value # all service instances String getServiceConfig(String name, String defaultValue) { String ret = null; // 1. look into in service config if(config.containsKey(name)) { ret = config.get(name) } // 2. look into Ranger admin config if(StringUtils.isBlank(ret)) { String[] configNames = new String[] { "ranger.service." + serviceName + "." + name, "ranger.servicetype." + svcType + "." + name, "ranger.service." + name, name }; for(String configName : configNames) { ret = PropertiesUtil.getProperty(configName); if(!StringUtils.isBlank(ret)) { break; } } } if(StringUtils.isBlank(ret)) { ret = defaultValue; } return ret; } security-admin/src/main/java/org/apache/ranger/common/PropertiesHelper.java <https://reviews.apache.org/r/34180/#comment135195> @author tag must go. Will be flagged as a violation. security-admin/src/main/java/org/apache/ranger/common/PropertiesHelper.java <https://reviews.apache.org/r/34180/#comment135194> Its best to avoid such wrapper classes, unless they add significant value. Less the code, the better it is - for maintainence, troubleshooting, etc. - Madhan Neethiraj On May 15, 2015, 11:21 p.m., Alok Lal wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34180/ > ----------------------------------------------------------- > > (Updated May 15, 2015, 11:21 p.m.) > > > Review request for ranger, Abhay Kulkarni, Madhan Neethiraj, and Ramesh Mani. > > > Bugs: RANGER-265 > https://issues.apache.org/jira/browse/RANGER-265 > > > Repository: ranger > > > Description > ------- > > Lookup and validation Calls to serices are done asynchrenously. Default > Timeout value is distributed with the default-site-config with a provision to > specify service-level override. > > > Diffs > ----- > > > agents-common/src/main/java/org/apache/ranger/plugin/service/ResourceLookupContext.java > 913f824 > security-admin/pom.xml 9783d1f > security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java > 009cbf8 > security-admin/src/main/java/org/apache/ranger/biz/ServiceMgr.java 8498fbf > > security-admin/src/main/java/org/apache/ranger/common/LookupConfigurator.java > PRE-CREATION > security-admin/src/main/java/org/apache/ranger/common/PropertiesHelper.java > PRE-CREATION > security-admin/src/main/java/org/apache/ranger/common/RangerFactory.java > PRE-CREATION > security-admin/src/main/java/org/apache/ranger/common/TimedExecutor.java > PRE-CREATION > > security-admin/src/main/java/org/apache/ranger/common/TimedExecutorConfigurator.java > PRE-CREATION > security-admin/src/main/java/org/apache/ranger/service/RangerFactory.java > 7834262 > security-admin/src/main/resources/conf.dist/ranger-admin-default-site.xml > 571d2a1 > > security-admin/src/test/java/org/apache/ranger/common/TestLookupConfigurator.java > PRE-CREATION > > security-admin/src/test/java/org/apache/ranger/common/TestTimedExecutor.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/34180/diff/ > > > Testing > ------- > > 1. Resouce lookup for hive, hdfs and hbase tested. Time taken to schedule > and execute the lookup calls looks fine. > 2. New test added to assert the executor behaviors around: > -- successful execution > -- timeout during or waiting for execution > -- rejection due to queue capacity limit > > > Thanks, > > Alok Lal > >
