----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18185/#review35436 -----------------------------------------------------------
jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java <https://reviews.apache.org/r/18185/#comment65958> lets track this TODO in a jira. It is not very useful comment here (ie not something like warning against an unimplemented part or so) jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java <https://reviews.apache.org/r/18185/#comment65957> It will be better to keep the position of createBinaryTransport and createHttpTransport same as before. That way the diff will be smaller and easier to read. Also, git blame will remain an useful tool for analyzing changes (it would be easier to find which line in createBinaryTransport changed when with it). jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java <https://reviews.apache.org/r/18185/#comment65959> this variable is not being used anywhere jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java <https://reviews.apache.org/r/18185/#comment65960> I am probably being too opinionated here! Feel free to disagree (if you do). I don't think we need this no-argument method, we can just use the method with single boolean argument. I think that will be more readable. jdbc/src/java/org/apache/hive/jdbc/HttpKerberosRequestInterceptor.java <https://reviews.apache.org/r/18185/#comment65976> Can you add a class level comment ? service/src/java/org/apache/hive/service/auth/HttpAuthHelper.java <https://reviews.apache.org/r/18185/#comment65989> can you add a class comment ?Something like "utility functions for http mode authentication". Maybe call this class HttpAuthUtils, so that its more clear what it contains ? service/src/java/org/apache/hive/service/auth/HttpCLIServiceProcessor.java <https://reviews.apache.org/r/18185/#comment65993> can you add a class comment ? service/src/java/org/apache/hive/service/auth/HttpCLIServiceUGIProcessor.java <https://reviews.apache.org/r/18185/#comment65994> can you add a class comment ? service/src/java/org/apache/hive/service/auth/HttpCLIServiceUGIProcessor.java <https://reviews.apache.org/r/18185/#comment66002> I think the better place to clear this is in ThriftHttpServlet, after the call to super.doPost(request, response), as it is set in the same place. shims/common-secure/src/main/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge20S.java <https://reviews.apache.org/r/18185/#comment65988> This is duplicating code in createClientTransport . Should we move this code to a static util class and re-use in both places ? shims/common/src/main/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge.java <https://reviews.apache.org/r/18185/#comment65987> wouldn't it be sufficient to use HadoopShims.getUGIForConf() instead of new method in thrift shims ? - Thejas Nair On Feb. 25, 2014, 12:23 p.m., Vaibhav Gumashta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18185/ > ----------------------------------------------------------- > > (Updated Feb. 25, 2014, 12:23 p.m.) > > > Review request for hive and Thejas Nair. > > > Bugs: HIVE-4764 > https://issues.apache.org/jira/browse/HIVE-4764 > > > Repository: hive-git > > > Description > ------- > > Support Kerberos HTTP authentication for HiveServer2 running in http mode > > > Diffs > ----- > > jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java 4102d7a > jdbc/src/java/org/apache/hive/jdbc/HttpBasicAuthInterceptor.java 66eba1b > jdbc/src/java/org/apache/hive/jdbc/HttpKerberosRequestInterceptor.java > PRE-CREATION > service/src/java/org/apache/hive/service/auth/HiveAuthFactory.java d8ba3aa > service/src/java/org/apache/hive/service/auth/HttpAuthHelper.java > PRE-CREATION > > service/src/java/org/apache/hive/service/auth/HttpAuthenticationException.java > PRE-CREATION > service/src/java/org/apache/hive/service/auth/HttpCLIServiceProcessor.java > PRE-CREATION > > service/src/java/org/apache/hive/service/auth/HttpCLIServiceUGIProcessor.java > PRE-CREATION > service/src/java/org/apache/hive/service/cli/CLIService.java 2b1e712 > service/src/java/org/apache/hive/service/cli/session/SessionManager.java > bfe0e7b > > service/src/java/org/apache/hive/service/cli/thrift/ThriftBinaryCLIService.java > 6fbc847 > service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java > 26bda5a > > service/src/java/org/apache/hive/service/cli/thrift/ThriftHttpCLIService.java > a6ff6ce > service/src/java/org/apache/hive/service/cli/thrift/ThriftHttpServlet.java > e77f043 > > shims/common-secure/src/main/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge20S.java > dc89de1 > shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java > 9e9a60d > > shims/common/src/main/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge.java > 03f4e51 > > Diff: https://reviews.apache.org/r/18185/diff/ > > > Testing > ------- > > > Thanks, > > Vaibhav Gumashta > >
