> On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 237 > > <https://reviews.apache.org/r/2975/diff/2/?file=61777#file61777line237> > > > > All properties that appear in HiveConf also need to appear in > > conf/hive-default.xml.template along with a description. > >
Done. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 238 > > <https://reviews.apache.org/r/2975/diff/2/?file=61777#file61777line238> > > > > > > I think it would make sense to change the name to > > 'hive.metastore.client.enable.setugi'. Also, I think this feature should be > > disabled by default. > > > > Done. False by default. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 239 > > <https://reviews.apache.org/r/2975/diff/2/?file=61777#file61777line239> > > > > Please add a new property hive.metastore.server.enable.setugi that > > allows this RPC to be disabled on the server side, and set the default > > value to false. I reused same config hive.metastore.execute.setugi at both client and server which is off by default. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/metastore/if/hive_metastore.thrift, line 438 > > <https://reviews.apache.org/r/2975/diff/2/?file=61778#file61778line438> > > > > Please add a comment explaining what this call does. Added comment. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ThriftHiveMetastore.java, > > line 145 > > <https://reviews.apache.org/r/2975/diff/2/?file=61782#file61782line145> > > > > When I apply your changes and run the thriftif ant target I see a small > > diff in this file. Did you use Thrift 0.7.0 to generate these files? I am not sure how that happened. I reran ant thriftif again. So, those should go away now. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, > > line 3589 > > <https://reviews.apache.org/r/2975/diff/2/?file=61787#file61787line3589> > > > > Indentation. Fixed. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, > > line 3743 > > <https://reviews.apache.org/r/2975/diff/2/?file=61787#file61787line3743> > > > > So instead of checking the hive.metastore.sasl.enabled property we now > > just check to see if we're using a security enabled shim, and if so assume > > that the user wants to enable security? I don't think this is correct > > behavior since the fact that we're using a secure version of Hadoop does > > not necessarily imply that we actually have security enabled. > > > > Also, it looks like this change deprecates the > > hive.metastore.sasl.enabled configuration property. In line with my comment > > above I think it makes sense to leave this property in, but if you do > > remove it then you need to release note the change and remove this property > > from HiveConf and conf/hive-default.xml.template. Reverted back to use old config variables to avoid the issues outlined. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, > > line 3751 > > <https://reviews.apache.org/r/2975/diff/2/?file=61787#file61787line3751> > > > > Indentation. Fixed. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, > > line 3756 > > <https://reviews.apache.org/r/2975/diff/2/?file=61787#file61787line3756> > > > > We're initializing SASL even if isSecure=false? Fixed. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, > > line 3758 > > <https://reviews.apache.org/r/2975/diff/2/?file=61787#file61787line3758> > > > > Formatting. Fixed. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java, > > line 263 > > <https://reviews.apache.org/r/2975/diff/2/?file=61788#file61788line263> > > > > Formatting: please add spaces. Fixed. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java, > > line 280 > > <https://reviews.apache.org/r/2975/diff/2/?file=61788#file61788line280> > > > > Should this be "Failed to login to the MetaStore Server..."? Fixed. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java, > > line 264 > > <https://reviews.apache.org/r/2975/diff/2/?file=61788#file61788line264> > > > > "new client talking to old metastore" seems to imply that we're able to > > determine whether or not we're talking to an old server, which isn't true. > > In reality, the onus is on the admin to ensure that both sides support this > > feature. What happens if the client calls set_ugi(), but the server doesn't > > support it? Is the error message helpful? If client calls set_ugi() and server doesn't support it then exception will be thrown to client which client will ignore and continue. So, effectively you will end up with old behavior. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/TUGIBasedProcessor.java, > > line 32 > > <https://reviews.apache.org/r/2975/diff/2/?file=61789#file61789line32> > > > > I think it's more accurate to say that the processor "*checks* to see > > if the first call is to set_ugi()..." instead of saying that it *expects* > > the first call to be to set_ugi(). Fixed. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/TUGIBasedProcessor.java, > > line 50 > > <https://reviews.apache.org/r/2975/diff/2/?file=61789#file61789line50> > > > > +1 to referencing the THRIFT JIRA. I think the class comment should > > call out that this is a temporary workaround cite a TODO. Done. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/TUGIBasedProcessor.java, > > line 92 > > <https://reviews.apache.org/r/2975/diff/2/?file=61789#file61789line92> > > > > Formatting: '} else {' Fixed. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/TUGIBasedProcessor.java, > > line 127 > > <https://reviews.apache.org/r/2975/diff/2/?file=61789#file61789line127> > > > > There's a TAB here. Please remove. Fixed. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/shims/src/0.20/java/org/apache/hadoop/hive/shims/Hadoop20Shims.java, > > line 519 > > <https://reviews.apache.org/r/2975/diff/2/?file=61791#file61791line519> > > > > Spacing. Fixed. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/shims/src/0.20S/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java, > > line 501 > > <https://reviews.apache.org/r/2975/diff/2/?file=61792#file61792line501> > > > > Please change the name of this method to isSecurityEnabled(), or create > > a new method with that name instead of modifying this method. The current > > name is misleading and will cause confusion (e.g. my comments in > > HiveMetaStore). Reverted this change as no longer required. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge20S.java, > > line 378 > > <https://reviews.apache.org/r/2975/diff/2/?file=61793#file61793line378> > > > > Spacing. Fixed. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/TUGIAssumingTransport.java, > > line 1 > > <https://reviews.apache.org/r/2975/diff/2/?file=61794#file61794line1> > > > > Missing ASF header. Added. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/TUGIAssumingTransport.java, > > line 16 > > <https://reviews.apache.org/r/2975/diff/2/?file=61794#file61794line16> > > > > If this is client-specific code then maybe the package should be > > o.a.h.hive.thrift.client. Moved. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/TUGIAssumingTransport.java, > > line 44 > > <https://reviews.apache.org/r/2975/diff/2/?file=61794#file61794line44> > > > > What's the point of these assert statements? Remove? Removed. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/shims/src/common/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge.java, > > line 78 > > <https://reviews.apache.org/r/2975/diff/2/?file=61796#file61796line78> > > > > Spacing. Added. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/shims/src/common/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge.java, > > line 83 > > <https://reviews.apache.org/r/2975/diff/2/?file=61796#file61796line83> > > > > Spacing. Fixed. > On 2011-12-16 03:06:59, Carl Steinbach wrote: > > trunk/shims/src/common/java/org/apache/hadoop/hive/thrift/TFilterTransport.java, > > line 1 > > <https://reviews.apache.org/r/2975/diff/2/?file=61797#file61797line1> > > > > ASF header. Added. - Ashutosh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2975/#review3939 ----------------------------------------------------------- On 2011-12-03 00:07:25, Ashutosh Chauhan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/2975/ > ----------------------------------------------------------- > > (Updated 2011-12-03 00:07:25) > > > 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 1209772 > trunk/metastore/if/hive_metastore.thrift 1209772 > trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.h 1209772 > trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.cpp 1209772 > > trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore_server.skeleton.cpp > 1209772 > > trunk/metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ThriftHiveMetastore.java > 1209772 > > trunk/metastore/src/gen/thrift/gen-php/hive_metastore/ThriftHiveMetastore.php > 1209772 > > trunk/metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore-remote > 1209772 > trunk/metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore.py > 1209772 > trunk/metastore/src/gen/thrift/gen-rb/thrift_hive_metastore.rb 1209772 > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > 1209772 > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java > 1209772 > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/TUGIBasedProcessor.java > PRE-CREATION > trunk/shims/ivy.xml 1209772 > trunk/shims/src/0.20/java/org/apache/hadoop/hive/shims/Hadoop20Shims.java > 1209772 > trunk/shims/src/0.20S/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java > 1209772 > > trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge20S.java > 1209772 > > trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/TUGIAssumingTransport.java > PRE-CREATION > trunk/shims/src/common/java/org/apache/hadoop/hive/shims/HadoopShims.java > 1209772 > > trunk/shims/src/common/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge.java > 1209772 > > 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 > >
