phoenix-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Josh Elser (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (PHOENIX-2535) Create shaded clients (thin + thick)
Date Fri, 08 Apr 2016 15:56:25 GMT

    [ https://issues.apache.org/jira/browse/PHOENIX-2535?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15232376#comment-15232376
] 

Josh Elser commented on PHOENIX-2535:
-------------------------------------

(sorry, this is getting long. Do we have reviewboard or something set up going forward?)

{code}
-  <packaging>pom</packaging>
+  <packaging>jar</packaging>
{code}

{code}
+            <id>default-jar</id>
+            <phase>none</phase>
+            <goals/>
{code}

Why make this change in phoenix-assembly/pom.xml? It seems like you're just working around
Maven wanting to create a jar then. If you set this to {{pom}}, AFAIUI, that should not be
trying to create any JAR for the module.

{code}
diff --git a/phoenix-assembly/src/build/components/all-common-jars.xml b/phoenix-assembly/src/build/components/all-common-jars.xml
index 960c3c9..f8c5abd 100644
--- a/phoenix-assembly/src/build/components/all-common-jars.xml
+++ b/phoenix-assembly/src/build/components/all-common-jars.xml
@@ -24,12 +24,17 @@
      <!-- Add the client & mapreduce jars. Expects the client jar packaging phase
to already be run,
       which is determined by specification order in the pom. -->
     <fileSet>
-      <directory>target</directory>
+      <directory>${project.basedir}/../phoenix-client/target</directory>
       <outputDirectory>/</outputDirectory>
       <includes>
         <include>phoenix-*-client.jar</include>
+      </includes>
+    </fileSet>
+    <fileSet>
+      <directory>${project.basedir}/../phoenix-server/target</directory>
+      <outputDirectory>/</outputDirectory>
+      <includes>
         <include>phoenix-*-server.jar</include>
-        <include>phoenix-*-mapreduce.jar</include>
       </includes>
     </fileSet>
     <fileSet>
{code}

Is there a reason why you aren't putting JARs generated by a module within that module's target/
directory? This is really confusing to see in practice "I built this module. Where the heck
are the artifacts?"

{code}
+                <relocation>
+                  <pattern>com.codahale</pattern>
+                  <shadedPattern>org.apache.phoenix.shaded.com.codahale</shadedPattern>
+                </relocation>
+                <relocation>
+                  <pattern>com.fasterxml</pattern>
+                  <shadedPattern>org.apache.phoenix.shaded.com.fasterxml</shadedPattern>
+                </relocation>
{code}

Might be a bit more concise to make a property instead of repeating the shaded relocation
prefix. E.g. {{<shaded.location>org.apache.phoenix.shaded</shaded.location>}}
and then {{<shadedPattern>$\{shaded.location\}.com.fasterxml</shadedPattern>}}.
Is this more concise?

{code}
+    <pluginManagement>
+      <plugins>
+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>
+          <artifactId>maven-shade-plugin</artifactId>
+      </plugin>
+    </plugins>
+    </pluginManagement>
{code}

This looks unnecessary. The maven-shade-plugin should already be defined in pluginManagement
in {{/pom.xml}}.

{code}
+  <dependencies>
+    <!-- Depend on all other internal projects -->
+    <dependency>
+      <groupId>org.apache.phoenix</groupId>
+      <artifactId>phoenix-core</artifactId>
+    </dependency>
+    <dependency>
+      <groupId>org.apache.phoenix</groupId>
+      <artifactId>phoenix-flume</artifactId>
+    </dependency>
+    <dependency>
+      <groupId>org.apache.phoenix</groupId>
+      <artifactId>phoenix-pig</artifactId>
+    </dependency>
+    <dependency>
+      <groupId>org.apache.phoenix</groupId>
+      <artifactId>phoenix-spark</artifactId>
+    </dependency>
+    <dependency>
+      <groupId>org.apache.phoenix</groupId>
+      <artifactId>phoenix-server</artifactId>
+    </dependency>
+    <dependency>
+      <groupId>org.apache.phoenix</groupId>
+      <artifactId>phoenix-server-client</artifactId>
+    </dependency>
+  </dependencies>
{code}

I'm surprised to see phoenix-server and phoenix-server-client in this list. My initial thought
was that phoenix-client would be a shaded jar for the thick driver. Either way, neither thick
or thin driver will need phoenix-server.

{code}
+    <module>phoenix-client</module>
{code}

Would it better to include the word "shaded" in the module name?

{code}
+              s
+              <transformer
{code}

A goof?

{code}
+            <transformers>
+              <transformer
+                  implementation="org.apache.maven.plugins.shade.resource.IncludeResourceTransformer">
+                <resource>NOTICE</resource>
+                <file>${project.basedir}/../../NOTICE</file>
+              </transformer>
...
+              <transformer
+                  implementation="org.apache.maven.plugins.shade.resource.IncludeResourceTransformer">
+                <resource>LICENSE.txt</resource>
+                <file>${project.basedir}/../../LICENSE.txt</file>
+              </transformer>
+            </transformers>
{code}

I hate to be the bearer of bad news, but these are most likely not meeting ASF licensing requirements.
For every bundled jar that Phoenix ships in a shaded jar, we're going to have to

# Copy any NOTICE text into a NOTICE file in the shaded jar
# Copy necessary license and copyright information for non-ASLv2 licensed jars into a LICENSE
file in the shaded jar.

Yes, this will be horrible, but it is required. 

> Create shaded clients (thin + thick) 
> -------------------------------------
>
>                 Key: PHOENIX-2535
>                 URL: https://issues.apache.org/jira/browse/PHOENIX-2535
>             Project: Phoenix
>          Issue Type: Bug
>            Reporter: Enis Soztutar
>            Assignee: Sergey Soldatov
>             Fix For: 4.8.0
>
>         Attachments: PHOENIX-2535-1.patch, PHOENIX-2535-2.patch, PHOENIX-2535-3.patch,
PHOENIX-2535-4.patch, PHOENIX-2535-5.patch
>
>
> Having shaded client artifacts helps greatly in minimizing the dependency conflicts at
the run time. We are seeing more of Phoenix JDBC client being used in Storm topologies and
other settings where guava versions become a problem. 
> I think we can do a parallel artifact for the thick client with shaded dependencies and
also using shaded hbase. For thin client, maybe shading should be the default since it is
new? 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message