You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2022/08/14 14:53:33 UTC
[GitHub] [drill] vvysotskyi opened a new pull request, #2622: DRILL-8279: Use thick Phoenix driver
vvysotskyi opened a new pull request, #2622:
URL: https://github.com/apache/drill/pull/2622
# [DRILL-8279](https://issues.apache.org/jira/browse/DRILL-8279): Use thick Phoenix driver
## Description
`phoenix-queryserver-client` shades Avatica classes, so it causes issues when starting Drill and shaded class from phoenix jars is loaded before official Avatica, so Drill wouldn't be able to start correctly.
- Fixed Phoenix unit tests to run with general tests suite without any extra checks
- Added support for Phoenix thick client
- Removed usage of custom dependencies
## Documentation
To use fat client, storage plugin configs should be updated as described in https://github.com/apache/drill/compare/master...vvysotskyi:DRILL-8279?expand=1#diff-81178bb7d4498bc22f7c54e71462b460f95b80f40f85e74ff8f1cc7f69d34351
## Testing
All unit tests pass, queried Phoenix using Drill.
--
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-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on pull request #2622: DRILL-8279: Use thick Phoenix driver
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on PR #2622:
URL: https://github.com/apache/drill/pull/2622#issuecomment-1216365721
@luocooong, Drill plugins are still pluggable, so you can provide your own implementations if you need.
Phoenix official connectors [1] for such big data tools like Spark, Hive and Pig also use thick client, so such decision should be production-suitable. By the way I didn't find official connectors that use thin client in that repository.
Ideally, if Phoenix thin client shades some libraries, it should also relocate them to avoid such issues. I don't see any other correct way for resolving this class path conflict in other way. Creating specific module and repacking Phoenix there when building Drill is overhead, and didn't guarantee that nothing would be broken. Having custom repo that provides relocated classes also not good decision since it will make more complex supporting new Phoenix versions.
[1] https://github.com/apache/phoenix-connectors
--
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-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton commented on pull request #2622: DRILL-8279: Use thick Phoenix driver
Posted by GitBox <gi...@apache.org>.
jnturton commented on PR #2622:
URL: https://github.com/apache/drill/pull/2622#issuecomment-1216187000
> Was there an agreement as to whether we should backport this to 1.20?
Not yet. For my part I'm supportive of backporting because I don't like the idea of a classloader time bomb ticking away in 1.20. As an alternative, 1.20 could remain on the thin driver but start distributing its own build of it with the needed Avatica relocations applied. But, since the thick driver is going to be preferred by most (all?) users, that seems like a wasted effort to me and I vote to backport this. Does anyone have a different take? @luocooong @vvysotskyi
--
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-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] luocooong commented on pull request #2622: DRILL-8279: Use thick Phoenix driver
Posted by GitBox <gi...@apache.org>.
luocooong commented on PR #2622:
URL: https://github.com/apache/drill/pull/2622#issuecomment-1216295980
**_Holy cow, that's unbelievable !_**
Why did this get approved without production experience?
Why do we need to force use of fat clients?
There are ways to resolve package conflicts, but this pull request directly lets me discard using Drill to query Phoenix !
--
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-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi merged pull request #2622: DRILL-8279: Use thick Phoenix driver
Posted by GitBox <gi...@apache.org>.
vvysotskyi merged PR #2622:
URL: https://github.com/apache/drill/pull/2622
--
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-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] Z0ltrix commented on pull request #2622: DRILL-8279: Use thick Phoenix driver
Posted by GitBox <gi...@apache.org>.
Z0ltrix commented on PR #2622:
URL: https://github.com/apache/drill/pull/2622#issuecomment-1214445771
Wow @vvysotskyi great work!
I'm so excited to see this in the real environment.
Regards
--
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-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a diff in pull request #2622: DRILL-8279: Use thick Phoenix driver
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on code in PR #2622:
URL: https://github.com/apache/drill/pull/2622#discussion_r946147460
##########
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:
For some reason CI tests fail with hadoop-2 profile, but pass fine locally. Seems like some issue with MiniKdc.
--
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-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton commented on pull request #2622: DRILL-8279: Use thick Phoenix driver
Posted by GitBox <gi...@apache.org>.
jnturton commented on PR #2622:
URL: https://github.com/apache/drill/pull/2622#issuecomment-1214844957
@vvysotskyi even though we've only seen this Avatica conflict affect Drill master after the Calcite upgrade was merged, do I understand correctly that it also lies latent the 1.20 branch? If so then my thinking is that this change should be backported even though it represents a type of breaking change for existing users of PQS. Anyone on 1.20.x who needs to continue with PQS to the extent that they are willing to roll the classloader dice (I'd like to hope that this is no one) could still manually replace their Phoenix plugin with a download of drill-storage-phoenix-1.20.2.jar, the last version to be based on PQS, or just remain stuck on 1.20.2 until they can coordinate an upgrade...
--
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-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on pull request #2622: DRILL-8279: Use thick Phoenix driver
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on PR #2622:
URL: https://github.com/apache/drill/pull/2622#issuecomment-1216459627
@luocooong, sorry, I didn't aim to outspeak overbearing, I just wanted to express my thoughts on why I didn't start a discussion on the mailing list.
If you have ideas on how to avoid this classpath issue and use phoenix-queryserver-client, feel free to suggest them or create a pull request with changes.
--
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-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on pull request #2622: DRILL-8279: Use thick Phoenix driver
Posted by GitBox <gi...@apache.org>.
cgivre commented on PR #2622:
URL: https://github.com/apache/drill/pull/2622#issuecomment-1216074207
Was there an agreement as to whether we should backport this to 1.20?
--
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-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] luocooong commented on pull request #2622: DRILL-8279: Use thick Phoenix driver
Posted by GitBox <gi...@apache.org>.
luocooong commented on PR #2622:
URL: https://github.com/apache/drill/pull/2622#issuecomment-1216372092
It was a real disappointment today.
As Java developers, all path conflicts will be solved in a way, but we chose one of the worst results.
What are we doing in such a hurry?
And I didn't see this pull request open for discussion before submission...
What does this mean for contributors?
--
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-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a diff in pull request #2622: DRILL-8279: Use thick Phoenix driver
Posted by GitBox <gi...@apache.org>.
cgivre commented on code in PR #2622:
URL: https://github.com/apache/drill/pull/2622#discussion_r946110147
##########
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:
See here: https://phoenix.apache.org/faq.html
```
Phoenix 4.x supports HBase 1.x running on Hadoop 2
Phoenix 5.x supports HBase 2.x running on Hadoop 3
```
--
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-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on pull request #2622: DRILL-8279: Use thick Phoenix driver
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on PR #2622:
URL: https://github.com/apache/drill/pull/2622#issuecomment-1216406330
@luocooong, at least I know several people that were already affected by this classpath issue, so it was the reason for fixing it quickly.
Jira ticket was created `14 Aug 14:36 EEST`, pull request was opened `14 Aug 17:53 EEST`, and merged `16 Aug 10:37 EEST`, so this time should be enough to participate in discussion or request some changes.
I didn't see a reason for creating a discussion in a mailing list for it, since plugin wasn't deleted, it is still functioning and even have improved developers experience by removing extra steps to run unit tests, enabling them in CI, and removing dependencies on custom repositories.
--
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-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton commented on a diff in pull request #2622: DRILL-8279: Use thick Phoenix driver
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on pull request #2622: DRILL-8279: Use thick Phoenix driver
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on PR #2622:
URL: https://github.com/apache/drill/pull/2622#issuecomment-1216258491
I agree that this change should be backported, since classpath issue might affect any user, even if user doesn't use Phoenix at all.
--
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-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton commented on pull request #2622: DRILL-8279: Use thick Phoenix driver
Posted by GitBox <gi...@apache.org>.
jnturton commented on PR #2622:
URL: https://github.com/apache/drill/pull/2622#issuecomment-1216343681
@luocooong let's see if we can offer storage plugins for both thick and thin drivers then? The classpath conflict bug in the thin driver is very serious, even if you have not yet been affected. It was important that some action was taken immediately.
Nothing has been released yet so we still have time to come with a path forward that works for everyone. Btw I thought I'd requested your review on this this PR as it was opened but I looking at the history I see must have failed to use the GH mobile app correctly so I do apologise if I only managed to bring this to your attention relatively late.
--
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-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] luocooong commented on pull request #2622: DRILL-8279: Use thick Phoenix driver
Posted by GitBox <gi...@apache.org>.
luocooong commented on PR #2622:
URL: https://github.com/apache/drill/pull/2622#issuecomment-1216419739
Sorry, I cannot accept the above overbearing statement.
I don't want to argue any more about this pull request, because that pull request was actually merged, so the right to speak is your right.
--
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-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org