[GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...
Github user jacques-n closed the pull request at: https://github.com/apache/drill/pull/116 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...
Github user dsbos commented on the pull request: https://github.com/apache/drill/pull/116#issuecomment-143913643 @jacques-n, can you close this pull request (given that that 4e3b7dc0333a01e72d0ea9256331ea1e1dd51181 is in)? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...
Github user jaltekruse commented on the pull request: https://github.com/apache/drill/pull/116#issuecomment-138649401 +1 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...
Github user dsbos commented on the pull request: https://github.com/apache/drill/pull/116#issuecomment-137811250 Would it be better if the JDBC-all Jar file didn't contain a logback.xml file? If the JDBC-all Jar doesn't already have that file, then to configure logback differently with their own logback.xml file, users only have to get their own file on the classpath _somewhere_. However, if the JDBC-all Jar does already have that file, it seems that users will have to get their logback.xml file on the class path _before_ the JDBC-all Jar file (right?). Although sometimes that's trivial, when the class path is set by, say, listing all .jar files in a lib/ directory (e.g., as in Tomcat for Spotfire), controlling the order might not be possible or easy. (A solution to keep the contents of the logback.xml file (as a convenient starting point for the user to copy out and modify) in the JDBC-all JAR file but not really have an active/interfering actual logback.xml file would be to use a modified name (e.g, logback-example.xml).) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/116#issuecomment-137827475 Agree on the logback. That shouldn't in there. An oversight on my part. We should be purely logging aganostic. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/116#issuecomment-137797991 I'm confused too. I got this comment in my email: "Something seems to be broken. After rebasing your branch on my branch with my DRILL-3347 (Hadoop Test) and DRILL-3566 (Prep.Stmt.) fixes, I tried installing the resulting JDBC-all Jar file on Spotfire, but Spotfire's getting IndexOutOfBoundsExceptions somewhere within ResultSet.next()." But I don't see it here. I was saying, lets make sure that this patch works as is before rebasing on 3347 and 3566. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...
Github user dsbos commented on the pull request: https://github.com/apache/drill/pull/116#issuecomment-137805930 Okay, now I think I see what happened. First of all, you can now ignore that "Something ... broken ..." comment; it's obsolete. (Your patch no longer seems broken.) (At first I thought things didn't work, and added that comment. Then I noticed that I had a local version/build mismatch, and amended the comment to say "hold on; I'm checking again," and tested again. Then everything worked, so I just deleted the comment from the GitHub review. Next time I'll amend rather than delete.) And now I recognize the "rebasing" reference: I didn't mean rebasing like rebasing on the latest version of master. I was just mentioning that rebasing was what I happened to use (as opposed to cherry-picking, merging, or other patching), in case my choice there caused the apparent breakage, to apply patches for DRILL-3347 and DRILL-3566 so I could try you patch with Spotfire (with would have hit the DRILL-3347 and DRILL-3566 bugs). So ... Your patch seems good; Spotfire ran fine with it. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/116#issuecomment-137621150 Can you start by reviewing my branch? Let's get it right before we try rebasing. With regards to slf4j and logging: I have corrected the behavior versus the old packaging. We support any slf4j logging tool but we don't package any. This means that a user can leverage their existing logging framework. If we include a logging framework, then a user can't use their own for centralized logging. We'll need to doc this but it is on purpose. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...
Github user dsbos commented on the pull request: https://github.com/apache/drill/pull/116#issuecomment-136902647 Regarding SLF4J and what should or shouldn't be included in the JDBC-all Jar file: Do we intend that someone using the JDBC-all Jar file can turn on Drill JDBC-layer logging by adding to the class path only a logback.xml file (that is, without adding additional Jar files/classes)? If so, then it seems like the JDBC-all Jar file is probably missing the logback-classic Jar file (which contains a org.slf4j.impl.StaticLoggerBinder). Or is it the case that we're leaving out back-end SLF4J libraries/classes on purpose (perhaps to avoid interfering with the user's choice of which back end to use)? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
Re: [GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...
I wrote: Github user dsbos commented on the pull request: https://github.com/apache/drill/pull/116#issuecomment-136889943 Something seems to be broken. Never mind that comment. I seemed to have a local version/build mismatch. Daniel After rebasing your branch on my branch with my DRILL-3347 (Hadoop Test) and DRILL-3566 (Prep.Stmt.) fixes, I tried installing the resulting JDBC-all Jar file on Spotfire, but Spotfire's getting IndexOutOfBoundsExceptions somewhere within ResultSet.next(). I'll see if I can identify which shading might be causing that or what's going on in next(). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- -- Daniel Barclay MapR Technologies
[GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...
Github user dsbos commented on the pull request: https://github.com/apache/drill/pull/116#issuecomment-136889943 Something seems to be broken. After rebasing your branch on my branch with my DRILL-3347 (Hadoop Test) and DRILL-3566 (Prep.Stmt.) fixes, I tried installing the resulting JDBC-all Jar file on Spotfire, but Spotfire's getting IndexOutOfBoundsExceptions somewhere within ResultSet.next(). I'll see if I can identify which shading might be causing that or what's going on in next(). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...
Github user dsbos commented on the pull request: https://github.com/apache/drill/pull/116#issuecomment-136890582 Another problem is that Logback logging might be broken (although maybe I just configured it wrong): When I added (on Spotfire's class path) a Jar file containing a logback.xml file in order to turn on logging, instead of getting log messages, I got these messages: SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder". SLF4J: Defaulting to no-operation (NOP) logger implementation SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details. My guess is that whatever module we intended to provide org.slf4j.impl.StaticLoggerBinder isn't included in the JDBC-all Jar file now. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...
Github user dsbos commented on the pull request: https://github.com/apache/drill/pull/116#issuecomment-131970908 It seems that you have shaded references to every Java package other than org.apache.drill.** (not just for every package from every depended-on artifact, but for _every_ package--including JDK packages). Note how the following command says it can't find oadd/java/sql/Driver: $ java -cp ./exec/jdbc-all/target/drill-jdbc-all-1.2.0-SNAPSHOT.jar org.apache.drill.jdbc.Driver Exception in thread main java.lang.NoClassDefFoundError: oadd/java/sql/Driver ... It seems that in trying to load org.apache.drill.jdbc.Driver it tried to load its base class, normally java.sql.Driver from the JDK, but the reference to that base class was shaded to oadd.org.java.sql.Driver, so of course it couldn't be found. (The expected behavior of that command is an error that org.apache.drill.jdbc.Driver doesn't have a main method.) What setup did you use to test the shading? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...
GitHub user jacques-n opened a pull request: https://github.com/apache/drill/pull/116 DRILL-3589: Update JDBC driver to shade and minimize dependencies. You can merge this pull request into a Git repository by running: $ git pull https://github.com/jacques-n/drill DRILL-3589 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/116.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #116 commit 84ff48855d6abb70f1cdffe27b6505810d6ff465 Author: Jacques Nadeau jacq...@apache.org Date: 2015-08-16T18:46:26Z DRILL-3589: Update JDBC driver to shade and minimize dependencies. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
