> On 2012-02-17 00:55:58, Ashutosh Chauhan wrote: > > src/java/org/apache/hcatalog/cli/SemanticAnalysis/HCatSemanticAnalyzer.java, > > line 130 > > <https://reviews.apache.org/r/3846/diff/3/?file=75536#file75536line130> > > > > Looks like this switch statement is no longer of any value now. Can > > this be removed?
The switch serves as to differentiate what is allowed and what is not. The default: case throws an exception. > On 2012-02-17 00:55:58, Ashutosh Chauhan wrote: > > src/java/org/apache/hcatalog/security/StorageDelegationAuthorizationProvider.java, > > line 43 > > <https://reviews.apache.org/r/3846/diff/3/?file=75540#file75540line43> > > > > Do we need to put class name of this class as a default value in > > conf/proto-hive-site.xml for authprovider conf value? There is two conf values: hive.security.authorization.enabled and hive.security.authorization.manager. I think we can default the authorization.manager, but I am not sure about defaulting authorization.enabled. If we also default that, then existing code and unit tests will be affected. > On 2012-02-17 00:55:58, Ashutosh Chauhan wrote: > > src/test/org/apache/hcatalog/cli/TestEximSemanticAnalysis.java.broken, line > > 1 > > <https://reviews.apache.org/r/3846/diff/3/?file=75543#file75543line1> > > > > Did you rename this file to disable the tests? You can just add name of > > this file in src/test/excluded-tests to exclude it, instead of renaming it. yes indeed. Did not notice we have src/test/excluded-tests. I'll add to that. > On 2012-02-17 00:55:58, Ashutosh Chauhan wrote: > > src/java/org/apache/hcatalog/cli/SemanticAnalysis/HCatSemanticAnalyzer.java, > > lines 263-264 > > <https://reviews.apache.org/r/3846/diff/3/?file=75536#file75536line263> > > > > This could potentially be an issue when table has lots of partitions in > > it. This will generate many listStatus call on NN in short time. Not sure > > if there is a way around that though. I guess, we can relax this check to look for table permissions. Agreed on the possible load generated by huge number of partitions. - enis ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3846/#review5175 ----------------------------------------------------------- On 2012-02-16 22:49:20, enis wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3846/ > ----------------------------------------------------------- > > (Updated 2012-02-16 22:49:20) > > > Review request for hcatalog. > > > Summary > ------- > > As per the design in the parent issue, we will delegate the authorization > checks to the storage handler (hdfs is considered as a storage handler as > well). This jira will introduce HiveAuthorizationProviders for hbase + hdfs. > > > This addresses bug HCATALOG-245. > https://issues.apache.org/jira/browse/HCATALOG-245 > > > Diffs > ----- > > src/java/org/apache/hcatalog/cli/SemanticAnalysis/AddPartitionHook.java > efbb79a > src/java/org/apache/hcatalog/cli/SemanticAnalysis/CreateDatabaseHook.java > 109de31 > src/java/org/apache/hcatalog/cli/SemanticAnalysis/CreateTableHook.java > 098a06b > src/java/org/apache/hcatalog/cli/SemanticAnalysis/HCatSemanticAnalyzer.java > 8387d8e > > src/java/org/apache/hcatalog/cli/SemanticAnalysis/HCatSemanticAnalyzerBase.java > PRE-CREATION > src/java/org/apache/hcatalog/common/AuthUtils.java 7cba8dc > src/java/org/apache/hcatalog/security/HdfsAuthorizationProvider.java > PRE-CREATION > > src/java/org/apache/hcatalog/security/StorageDelegationAuthorizationProvider.java > PRE-CREATION > src/test/org/apache/hcatalog/HcatTestUtils.java PRE-CREATION > src/test/org/apache/hcatalog/cli/TestEximSemanticAnalysis.java 64bde1b > src/test/org/apache/hcatalog/cli/TestEximSemanticAnalysis.java.broken > PRE-CREATION > src/test/org/apache/hcatalog/security/TestHdfsAuthorizationProvider.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/3846/diff > > > Testing > ------- > > > Thanks, > > enis > >
