stoty commented on a change in pull request #1239:
URL: https://github.com/apache/phoenix/pull/1239#discussion_r639021914
##########
File path: phoenix-assembly/src/build/package-to-tar-all.xml
##########
@@ -30,8 +30,23 @@
</formats>
<includeBaseDirectory>true</includeBaseDirectory>
+ <!-- Components that we don't want in jars that are used with other
libraries, but we want for a standalone client -->
+ <dependencySets>
+ <dependencySet>
+ <!-- Unpack all the dependencies to class files, since java doesn't
support
+ jar of jars for running -->
+ <unpack>false</unpack>
+ <!-- save these dependencies to the top-level -->
Review comment:
This comment is just false
##########
File path: phoenix-core/pom.xml
##########
@@ -523,6 +523,13 @@
<artifactId>hamcrest-core</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
Review comment:
Why is this needed ?
Ideally Phoenix-core doesn't have anything to do with sqlline.
##########
File path: phoenix-assembly/pom.xml
##########
@@ -132,6 +132,10 @@
<groupId>org.apache.phoenix</groupId>
<artifactId>phoenix-client-${hbase.suffix}</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.phoenix</groupId>
+ <artifactId>phoenix-client-embedded-${hbase.suffix}</artifactId>
+ </dependency>
Review comment:
I don't think we should to add the standard (old) client to the assembly
after this change.
##########
File path: phoenix-assembly/src/build/package-to-tar-all.xml
##########
@@ -30,8 +30,23 @@
</formats>
<includeBaseDirectory>true</includeBaseDirectory>
+ <!-- Components that we don't want in jars that are used with other
libraries, but we want for a standalone client -->
+ <dependencySets>
+ <dependencySet>
+ <!-- Unpack all the dependencies to class files, since java doesn't
support
Review comment:
I don't think this comment is relevant to anything anymore.
##########
File path: bin/phoenix_utils.py
##########
@@ -77,10 +77,13 @@ def findClasspath(command_name):
return tryDecode(subprocess.Popen(command, shell=True,
stdout=subprocess.PIPE).stdout.read())
def setPath():
- PHOENIX_CLIENT_JAR_PATTERN = "phoenix-client-hbase-*[!s].jar"
+ PHOENIX_CLIENT_EMBEDDED_JAR_PATTERN =
"phoenix-client-embedded-hbase-*[!s].jar"
Review comment:
phoenix-client-embedded is missing the SLF4J backend as well.
It should handled similarly to the sqlline JAR, with loading it from /lib by
default, but having an option to override (and use a different backed JAR)
##########
File path: phoenix-client-parent/phoenix-client-embedded/pom.xml
##########
@@ -81,10 +81,5 @@
<artifactId>phoenix-hbase-compat-${hbase.compat.version}</artifactId>
<optional>false</optional>
</dependency>
- <dependency>
Review comment:
I thought that we are already not adding sqlline here.
Just noting that we change sqlline-embedded too.
##########
File path: phoenix-client-parent/phoenix-client/pom.xml
##########
@@ -85,10 +85,5 @@
<artifactId>log4j</artifactId>
<scope>runtime</scope>
</dependency>
- <dependency>
Review comment:
I think that we'd best keep the old client unchanged for backwards
compatibility reasons.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
