Phoenix 6377

[stoty]

No description provided.

[dbwong]

Probably not entirely related to this but why do we need sqlline as a runtime dependency in the client?

[stoty]

We are traditionally including sqlline in phoenix-client for the sqlline.py client.

I wouldn't be against unbundling it, in fact, I have already done that for the thin client.
(We'd have to reintroduce a lib dir, or ship sqlline in the root dir, though)

[dbwong]

Lets unbundle in another jira (is there one already?) as it is not really a runtime dependency for the client. One thing we might want to consider is an import dependency so we can share a single pom across all of these client flavours. Any thoughts? EDIT: I guess it is not clear from the JIRA or here why we are pushing the dependency down.

[stoty]

One thing we might want to consider is an import dependency so we can share a single pom across all of these client flavours. Any thoughts?

I don't really get what you mean. We are already have most common setting in the parent pom. Can you add a little more detail ?

EDIT: I guess it is not clear from the JIRA or here why we are pushing the dependency down.

It is not clear to me either :(
Having the dependencies in the parent pom results in having all the shaded dependencies erronously added to the client artifact descriptor.
Moving the dependecies one level down results in an artifact with no dependencies (the expected behaviour)

Not the first and not the last maven bug (or undocumented/unexplained behaviour) that we see.
Unfortunately, we didn't catch this one until after 5.1.0 was released.

[stoty]

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 31s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 1s	No case conflicting files found.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
-1 ❌	test4tests	0m 0s	The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
		_ master Compile Tests _
+0 🆗	mvndep	4m 31s	Maven dependency ordering for branch
+1 💚	mvninstall	10m 58s	master passed
+0	hbaserecompile	22m 10s	HBase recompiled.
+1 💚	compile	1m 23s	master passed
+1 💚	javadoc	0m 50s	master passed
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 11s	Maven dependency ordering for patch
+1 💚	mvninstall	8m 2s	the patch passed
+0	hbaserecompile	14m 19s	HBase recompiled.
+1 💚	compile	1m 9s	the patch passed
+1 💚	javac	1m 9s	the patch passed
-1 ❌	whitespace	0m 0s	The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
+1 💚	xml	0m 4s	The patch has no ill-formed XML file.
+1 💚	javadoc	0m 50s	the patch passed
		_ Other Tests _
+1 💚	unit	0m 16s	phoenix-client-parent in the patch passed.
+1 💚	unit	5m 44s	phoenix-client in the patch passed.
+1 💚	unit	5m 41s	phoenix-client-embedded in the patch passed.
+0 🆗	asflicense	0m 42s	ASF License check generated no output?
		55m 5s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1136/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#1136
Optional Tests	dupname asflicense javac javadoc unit xml compile
uname	Linux 715ada1b8885 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev/phoenix-personality.sh
git revision	master / 9e90779
Default Java	Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08
whitespace	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1136/1/artifact/yetus-general-check/output/whitespace-eol.txt
Test Results	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1136/1/testReport/
Max. process+thread count	96 (vs. ulimit of 30000)
modules	C: phoenix-client-parent phoenix-client-parent/phoenix-client phoenix-client-parent/phoenix-client-embedded U: phoenix-client-parent
Console output	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1136/1/console
versions	git=2.7.4 maven=3.3.9
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

[dbwong]

I don't really get what you mean. We are already have most common setting in the parent pom. Can you add a little more detail ?

I was referring to an approach like this https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html#Importing_Dependencies. But I was trying to understand why the client parent couldn't just be that other pom.

[stoty]

OK, now I understand what you mean.
We're getting everything but sqlline and the hbase compatibility module from phoenix-core.
I don't really think that adding another mechanism to avoid duplicating two or three dependecies across two modules helps a lot with maintainability, and having just been burned by maven I am happy just to have a version that works.

However, If you can make it work, then by all means open another ticket or an alternative PR with that solution.

[dbwong]

+1 I think i'd prefer to fix whatever is wrong with the shading to the approach I mentioned anyway.

[stoty]

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 31s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
-1 ❌	test4tests	0m 0s	The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
		_ master Compile Tests _
+0 🆗	mvndep	4m 31s	Maven dependency ordering for branch
+1 💚	mvninstall	10m 58s	master passed
+0	hbaserecompile	22m 14s	HBase recompiled.
+1 💚	compile	1m 24s	master passed
+1 💚	javadoc	0m 50s	master passed
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 11s	Maven dependency ordering for patch
+1 💚	mvninstall	8m 3s	the patch passed
+0	hbaserecompile	14m 24s	HBase recompiled.
+1 💚	compile	1m 10s	the patch passed
+1 💚	javac	1m 10s	the patch passed
-1 ❌	whitespace	0m 0s	The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
+1 💚	xml	0m 4s	The patch has no ill-formed XML file.
+1 💚	javadoc	0m 47s	the patch passed
		_ Other Tests _
+1 💚	unit	0m 16s	phoenix-client-parent in the patch passed.
+1 💚	unit	5m 42s	phoenix-client in the patch passed.
+1 💚	unit	5m 41s	phoenix-client-embedded in the patch passed.
+0 🆗	asflicense	0m 42s	ASF License check generated no output?
		55m 10s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1136/2/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#1136
Optional Tests	dupname asflicense javac javadoc unit xml compile
uname	Linux 7f01459a1db4 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev/phoenix-personality.sh
git revision	master / 9e90779
Default Java	Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08
whitespace	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1136/2/artifact/yetus-general-check/output/whitespace-eol.txt
Test Results	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1136/2/testReport/
Max. process+thread count	96 (vs. ulimit of 30000)
modules	C: phoenix-client-parent phoenix-client-parent/phoenix-client phoenix-client-parent/phoenix-client-embedded U: phoenix-client-parent
Console output	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1136/2/console
versions	git=2.7.4 maven=3.3.9
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.