jnturton commented on code in PR #2622: URL: https://github.com/apache/drill/pull/2622#discussion_r945557835
########## contrib/storage-phoenix/README.md: ########## @@ -83,11 +83,11 @@ Tips : Configurations : 1. Enable [Drill User Impersonation](https://drill.apache.org/docs/configuring-user-impersonation/) 2. Enable [PQS Impersonation](https://phoenix.apache.org/server.html#Impersonation) -3. PQS URL: - 1. Provide `host` and `port` and Drill will generate the PQS URL with a doAs parameter of current session user - 2. Provide the `jdbcURL` with a `doAs` url param and `$user` placeholder as a value, for instance: - `jdbc:phoenix:thin:url=http://localhost:8765?doAs=$user`. In case Drill Impersonation is enabled, but `doAs=$user` - is missing the User Exception is thrown. +3. Phoenix URL: + 1. Provide `zkQUorum` and `port` and Drill will create a connection to Phoenix with a doAs of current Review Comment: ```suggestion 1. Provide `zkQuorum` and `port` and Drill will create a connection to Phoenix with a doAs of current ``` ########## contrib/storage-phoenix/pom.xml: ########## @@ -353,6 +381,9 @@ <profiles> <profile> <id>hadoop-2</id> + <properties> + <phoenix.skip.tests>true</phoenix.skip.tests> Review Comment: Is the Phoenix thick driver only compatible with Hadoop 3? ########## contrib/storage-phoenix/src/main/java/org/apache/drill/exec/store/phoenix/PhoenixDataSource.java: ########## @@ -18,40 +18,42 @@ package org.apache.drill.exec.store.phoenix; import java.io.PrintWriter; +import java.security.PrivilegedExceptionAction; import java.sql.Connection; import java.sql.DriverManager; import java.sql.SQLException; import java.util.Map; +import java.util.Optional; import java.util.Properties; +import java.util.function.Supplier; import java.util.logging.Logger; import javax.sql.DataSource; -import org.apache.drill.common.exceptions.UserException; +import org.apache.drill.common.exceptions.DrillRuntimeException; +import org.apache.drill.common.util.function.CheckedSupplier; +import org.apache.drill.exec.util.ImpersonationUtil; import org.apache.drill.shaded.guava.com.google.common.base.Preconditions; +import org.apache.hadoop.security.UserGroupInformation; import org.slf4j.LoggerFactory; /** * Phoenix’s Connection objects are different from most other JDBC Connections * due to the underlying HBase connection. The Phoenix Connection object * is designed to be a thin object that is inexpensive to create. - * + * <p> * If Phoenix Connections are reused, it is possible that the underlying HBase connection * is not always left in a healthy state by the previous user. It is better to * create new Phoenix Connections to ensure that you avoid any potential issues. Review Comment: Reviewer's note: I checked the Phoenix FAQ and believe that these comments remain true of the thick driver. ########## contrib/storage-phoenix/src/main/java/org/apache/drill/exec/store/phoenix/PhoenixDataSource.java: ########## @@ -138,30 +137,37 @@ public boolean isWrapperFor(Class<?> iface) { @Override public Connection getConnection() throws SQLException { - useDriverClass(); + loadDriverClass(); return getConnection(this.user, null); } @Override public Connection getConnection(String userName, String password) throws SQLException { - useDriverClass(); + loadDriverClass(); logger.debug("Drill/Phoenix connection url: {}", url); - return DriverManager.getConnection(url, useConfProperties()); + CheckedSupplier<Connection, SQLException> action = + () -> DriverManager.getConnection(url, useConfProperties()); + if (impersonationEnabled) { + return doAsRemoteUser(userName, action); + } + return action.getAndThrow(); + } + + private <T> T doAsRemoteUser(String remoteUserName, final Supplier<T> action) { + try { + UserGroupInformation proxyUser = ImpersonationUtil.createProxyUgi(remoteUserName); + return proxyUser.doAs((PrivilegedExceptionAction<T>) action::get); + } catch (Exception e) { + throw new DrillRuntimeException(e); + } } /** - * The thin-client is lightweight and better compatibility. - * Only thin-client is currently supported. - * - * @throws SQLException + * Only thick-client is currently supported doe to shaded Avatica issues with thin client. Review Comment: ```suggestion * Only thick-client is currently supported due to a shaded Avatica conflict created by the thin client. ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
