New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PHOENIX-6378 Unbundle sqlline from phoenix-client #1239
Conversation
<!-- 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 --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is just false
<!-- 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this comment is relevant to anything anymore.
@@ -85,10 +85,5 @@ | |||
<artifactId>log4j</artifactId> | |||
<scope>runtime</scope> | |||
</dependency> | |||
<dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we'd best keep the old client unchanged for backwards compatibility reasons.
phoenix-core/pom.xml
Outdated
@@ -523,6 +523,13 @@ | |||
<artifactId>hamcrest-core</artifactId> | |||
<scope>test</scope> | |||
</dependency> | |||
<dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed ?
Ideally Phoenix-core doesn't have anything to do with sqlline.
@@ -81,10 +81,5 @@ | |||
<artifactId>phoenix-hbase-compat-${hbase.compat.version}</artifactId> | |||
<optional>false</optional> | |||
</dependency> | |||
<dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that we are already not adding sqlline here.
Just noting that we change sqlline-embedded too.
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
e96b365
to
bd98119
Compare
💔 -1 overall
This message was automatically generated. |
Could you update the ticket title and commit message ? |
bd98119
to
a60ddf9
Compare
bin/phoenix_utils.py
Outdated
PHOENIX_TRACESERVER_JAR_PATTERN = "phoenix-tracing-webapp-*-runnable.jar" | ||
PHOENIX_TESTS_JAR_PATTERN = "phoenix-core-*-tests*.jar" | ||
PHOENIX_PHERF_JAR_PATTERN = "phoenix-pherf-*[!s].jar" | ||
SLF4J_JAR_PATTERN = "slf4j-log4j12-*[!s].jar" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change the slf4j* variable names to slf4j_log4j12_* for clarity.
bin/sqlline.py
Outdated
@@ -108,7 +108,9 @@ def kill_child(): | |||
colorSetting = "false" | |||
|
|||
java_cmd = java + ' $PHOENIX_OPTS ' + \ | |||
' -cp "' + hbase_config_path + os.pathsep + phoenix_utils.hbase_conf_dir + os.pathsep + phoenix_utils.phoenix_client_jar + \ | |||
' -cp "' + phoenix_utils.sqlline_with_deps_jar + os.pathsep + hbase_config_path + os.pathsep + \ | |||
phoenix_utils.slf4j_jar + os.pathsep + \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this variable name hasn't been updated.
bin/phoenix_utils.py
Outdated
if sqlline_with_deps_jar is None or sqlline_with_deps_jar == "": | ||
sqlline_with_deps_jar = findFileInPathWithoutRecursion(SQLLINE_WITH_DEPS_PATTERN, os.path.join(current_dir, "..","lib")) | ||
|
||
global slf4j_jar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this variable name hasn't been updated
bin/phoenix_utils.py
Outdated
@@ -216,3 +232,4 @@ def common_sqlline_args(parser): | |||
print("phoenix_loadbalancer_jar:", phoenix_loadbalancer_jar) | |||
print("phoenix_thin_client_jar:", phoenix_thin_client_jar) | |||
print("hadoop_classpath:", hadoop_classpath) | |||
print("sqlline_with_deps_jar", sqlline_with_deps_jar) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should log the slf4j_log4j12 jar name too.
f7a24af
to
6c064dd
Compare
bin/phoenix_utils.py
Outdated
SQLLINE_WITH_DEPS_PATTERN = "sqlline-*-jar-with-dependencies.jar" | ||
|
||
|
||
OVERRIDE_SLF4J = "OVERRIDE_SLF4J_JAR_LOCATION" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, we should call it SLF4J_BACKEND_JAR instead of SLF4J_LOG4J12_JAR.
and apply it everywhere, including the name of the OVERRIDE system property.
6c064dd
to
6a825ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 LGTM
…it in sqlline.py Change-Id: Ifd31e25b303ab3d2872b53f4a503219539777544
6a825ef
to
8ce80c3
Compare
Thanks @stoty for the review. I squashed the commits to 1, before merging. |
💔 -1 overall
This message was automatically generated. |
Change-Id: I443593c18d412c4ccd2e137c6b2809863bc71bc8