Re: Review Request 66081: Kerberos authentication in lens

[Ankit Kailaswar <an...@gmail.com>]

> On March 20, 2018, 6:37 a.m., Rajat Khandelwal wrote:
> > lens-driver-hive/src/main/java/org/apache/lens/driver/hive/RetryingThriftCLIServiceClientSasl.java
> > Lines 125 (patched)
> > <https://reviews.apache.org/r/66081/diff/1/?file=1976384#file1976384line125>
> >
> >     This class is also redundant. Can we reuse `org.apache.hive.service.cli.thrift.RetryingThriftCLIServiceClient.InvocationResult`?

it is protected inner class, usage are scoped within package only. Wont be a good idea to follow package struture for single class in lens. cant even extend outside package, no default ctor available for InvocationResult in RetryingThriftCLIServiceClient.


> On March 20, 2018, 6:37 a.m., Rajat Khandelwal wrote:
> > lens-driver-hive/src/main/java/org/apache/lens/driver/hive/RetryingThriftCLIServiceClientSasl.java
> > Lines 137-182 (patched)
> > <https://reviews.apache.org/r/66081/diff/1/?file=1976384#file1976384line137>
> >
> >     Both these methods can be easily avoided by simply extending `RetryingThriftCLIServiceClient`.

it can't be extended form RetryingThriftCLIServiceClient please read above reply to puneet's comment.


On March 20, 2018, 6:37 a.m., Ankit Kailaswar wrote:
> > Current design:
> > ```java
> > HiveConf conf;
> > if (conf.auth == Kerberose) {
> > 	org.apache.lens.driver.hive.RetryingThriftCLIServiceClientSasl.newClient
> > } else {
> > 	org.apache.hive.service.cli.thrift.RetryingThriftCLIServiceClient.newClient
> > }
> > 
> > 
> > class RetryingThriftCLIServiceClientSasl {
> > 	// some reuse from RetryingThriftCLIServiceClient
> > 	// some code copied from RetryingThriftCLIServiceClient
> > }
> > 
> > ```
> > 
> > Proposal: 
> > 
> > 
> > ```java
> > HiveConf conf;
> > 
> > org.apache.lens.driver.hive.client.thrift.RetryingThriftCLIServiceClient.newClient
> > 
> > class RetryingThriftCLIServiceClient extends org.apache.hive.service.cli.thrift.RetryingThriftCLIServiceClient {
> > 	@Override
> > 	protected synchronized TTransport connect(HiveConf conf) throws HiveSQLException, TTransportException {
> >         if (this.transport != null && this.transport.isOpen()) {
> >             this.transport.close();
> >         }
> > 
> >         String host = conf.getVar(ConfVars.HIVE_SERVER2_THRIFT_BIND_HOST);
> >         int port = conf.getIntVar(ConfVars.HIVE_SERVER2_THRIFT_PORT);
> >         LOG.info("Connecting to " + host + ":" + port);
> >         this.transport = new TSocket(host, port);
> >         ((TSocket)this.transport).setTimeout((int)conf.getTimeVar(ConfVars.SERVER_READ_SOCKET_TIMEOUT, TimeUnit.SECONDS) * 1000);
> > 
> >         try {
> >             ((TSocket)this.transport).getSocket().setKeepAlive(conf.getBoolVar(ConfVars.SERVER_TCP_KEEP_ALIVE));
> >         } catch (SocketException var8) {
> >             LOG.error("Error setting keep alive to " + conf.getBoolVar(ConfVars.SERVER_TCP_KEEP_ALIVE), var8);
> >         }
> >        	/* Move the following code to new method getTransport below in the else part: 
> >         String userName = conf.getVar(ConfVars.HIVE_SERVER2_THRIFT_CLIENT_USER);
> >         String passwd = conf.getVar(ConfVars.HIVE_SERVER2_THRIFT_CLIENT_PASSWORD);
> > 
> >         try {
> >             this.transport = PlainSaslHelper.getPlainTransport(userName, passwd, this.transport);
> >         } catch (SaslException var7) {
> >             LOG.error("Error creating plain SASL transport", var7);
> >         }
> >         */
> >         this.transport = getTransport(conf);
> >         TProtocol protocol = new TBinaryProtocol(this.transport);
> >         this.transport.open();
> >         this.base = new ThriftCLIServiceClient(new Client(protocol));
> >         LOG.info("Connected!");
> >         return this.transport;
> >     }
> >     protected TTransport getTransport(HiveConf conf) {
> >     	if (conf.auth == Kerberose) {
> >     		// use KerberoseSaslHelper
> >     	} else {
> >     		// use PlainSaslHelper
> >     	}
> >     }
> > }
> > 
> > ```

member variables of base class cant be accesses from derived class.


- Ankit


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66081/#review199524
-----------------------------------------------------------


On March 20, 2018, 8:43 p.m., Ankit Kailaswar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66081/
> -----------------------------------------------------------
> 
> (Updated March 20, 2018, 8:43 p.m.)
> 
> 
> Review request for lens, Amareshwari Sriramadasu, Rajat Khandelwal, and Puneet Gupta.
> 
> 
> Repository: lens
> 
> 
> Description
> -------
> 
> https://issues.apache.org/jira/browse/LENS-1506
> 
> This patch contains code changes to enable kerberos authentication for 
> 1. lens to hive 
> 2. lens to metastore
> 3. lens to hdfs
> 
> code changes are as follows,
> 1. new http thrift client for hive driver to support sasl transport for kerberozied hive server.
> 2. cron to update KDC ticket before it expires.
> 
> 
> Diffs
> -----
> 
>   lens-driver-hive/src/main/java/org/apache/lens/driver/hive/HiveDriver.java 2eb94aa7 
>   lens-driver-hive/src/main/java/org/apache/lens/driver/hive/RemoteThriftConnection.java 54885f77 
>   lens-driver-hive/src/main/java/org/apache/lens/driver/hive/RetryingThriftCLIServiceClientSasl.java PRE-CREATION 
>   lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java d5273be8 
>   lens-server/src/main/java/org/apache/lens/server/BaseLensService.java b9fcdd8b 
>   lens-server/src/main/java/org/apache/lens/server/session/HiveSessionService.java 31ac358d 
> 
> 
> Diff: https://reviews.apache.org/r/66081/diff/2/
> 
> 
> Testing
> -------
> 
> unit testing
> 
> 
> Thanks,
> 
> Ankit Kailaswar
> 
>