----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2975/#review3984 -----------------------------------------------------------
Is it possible to add some testcases? Specifically I'd like to see a test that has a client with setugi enabled connect to a server with setugi disabled and vice-versa. trunk/conf/hive-default.xml.template <https://reviews.apache.org/r/2975/#comment9004> This describes the action instead of the effect. Please change to something like "In unsecure mode, setting this property to true will cause the metastore to execute DFS operations using the client's reported user and group permissions. Note that this property must be set on both the client and server sides." Also, it may be easier to understand if you separate this out into to separate properties: hive.metastore.client.setugi and hive.metastore.server.setugi trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java <https://reviews.apache.org/r/2975/#comment9005> Spacing. trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java <https://reviews.apache.org/r/2975/#comment9000> The formatting/indentation in this method is still not correct. Please use 2 character indents, nested 'else' operators, etc. trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java <https://reviews.apache.org/r/2975/#comment9009> Please add some logging statements here, e.g. "Starting DB backed MetaStore Server in Secure Mode" "Starting DB backed MetaStore Server" "Starting DB backed MetaStore Server with SetUGI enabled" trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java <https://reviews.apache.org/r/2975/#comment9001> If the call to set_ugi() fails, is logging a message and continuing really the right behavior? Why not just fail outright? Also, if you think that continuing is the correct behavior, then I think the description of metastore.execute.setugi should be updated to explain that this is a best effort approach, and it's possible that your setting will not be honored. trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java <https://reviews.apache.org/r/2975/#comment8999> There's a TAB character here and on line 285. Please remove. trunk/shims/src/common/java/org/apache/hadoop/hive/shims/HadoopShims.java <https://reviews.apache.org/r/2975/#comment9013> Add a newline. - Carl On 2011-12-17 02:42:36, Ashutosh Chauhan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/2975/ > ----------------------------------------------------------- > > (Updated 2011-12-17 02:42:36) > > > Review request for hive. > > > Summary > ------- > > Pass user identity in metastore connection in unsecure mode > > > This addresses bug HIVE-2616. > https://issues.apache.org/jira/browse/HIVE-2616 > > > Diffs > ----- > > trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 1215380 > trunk/conf/hive-default.xml.template 1215380 > trunk/metastore/if/hive_metastore.thrift 1215380 > trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.h 1215380 > trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.cpp 1215380 > > trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore_server.skeleton.cpp > 1215380 > > trunk/metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ThriftHiveMetastore.java > 1215380 > > trunk/metastore/src/gen/thrift/gen-php/hive_metastore/ThriftHiveMetastore.php > 1215380 > > trunk/metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore-remote > 1215380 > trunk/metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore.py > 1215380 > trunk/metastore/src/gen/thrift/gen-rb/thrift_hive_metastore.rb 1215380 > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > 1215380 > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java > 1215380 > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/TUGIBasedProcessor.java > PRE-CREATION > trunk/shims/ivy.xml 1215380 > trunk/shims/src/0.20/java/org/apache/hadoop/hive/shims/Hadoop20Shims.java > 1215380 > trunk/shims/src/0.20S/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java > 1215380 > > trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge20S.java > 1215380 > > trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/client/TUGIAssumingTransport.java > PRE-CREATION > trunk/shims/src/common/java/org/apache/hadoop/hive/shims/HadoopShims.java > 1215380 > > trunk/shims/src/common/java/org/apache/hadoop/hive/thrift/TFilterTransport.java > PRE-CREATION > > trunk/shims/src/common/java/org/apache/hadoop/hive/thrift/TUGIContainingTransport.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/2975/diff > > > Testing > ------- > > All the tests in metastore dir passes. Manually tested that file on hdfs is > owned by user running the client and not by user running metastore server. > > > Thanks, > > Ashutosh > >
