PHOENIX-6193 PHOENIX-6151 slows down shading #925

stoty · 2020-10-17T08:56:04Z

No description provided.

stoty · 2020-10-17T10:15:17Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 11s	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 _
-1 ❌	mvninstall	47m 35s	root in master failed.
+1 💚	compile	0m 34s	master passed
+1 💚	javadoc	0m 13s	master passed
		_ Patch Compile Tests _
-1 ❌	mvninstall	15m 19s	root in the patch failed.
+1 💚	compile	0m 27s	the patch passed
+1 💚	javac	0m 27s	the patch passed
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	xml	0m 1s	The patch has no ill-formed XML file.
+1 💚	javadoc	0m 11s	the patch passed
		_ Other Tests _
+1 💚	unit	12m 7s	phoenix-client in the patch passed.
+1 💚	asflicense	0m 11s	The patch does not generate ASF License warnings.
		78m 26s

Subsystem	Report/Notes
Docker	ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-925/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#925
Optional Tests	dupname asflicense javac javadoc unit xml compile
uname	Linux b5e26355b6b7 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev/phoenix-personality.sh
git revision	master / `1784848`
Default Java	Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08
mvninstall	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-925/1/artifact/yetus-general-check/output/branch-mvninstall-root.txt
mvninstall	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-925/1/artifact/yetus-general-check/output/patch-mvninstall-root.txt
Test Results	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-925/1/testReport/
Max. process+thread count	63 (vs. ulimit of 30000)
modules	C: phoenix-client U: phoenix-client
Console output	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-925/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.

lhofhansl · 2020-10-17T20:58:13Z

I'll try this out today.

lhofhansl · 2020-10-17T21:11:35Z

Faster, about 500KB/s, but still take a long time to build the two 150MB jars.
It keeps one core busy on my machine... Can we build client and client-embedded in parallel?
(I'm building with -T 8, but these two jars are build in the same task)

stoty · 2020-10-18T09:10:56Z

I've re-run the build with warm maven caches:
Pre-6151:
[INFO] Phoenix Client ..................................... SUCCESS [02:20 min]
With this patch:
[INFO] Phoenix Client ..................................... SUCCESS [05:10 min]

So we're still about twice as slow as the original shading setup. Part of that is probably simply that we are shading more stuff, but using excludes probably also contributes.

Putting the two client JARs in different modules is certainly possible, however I did not have much luck running the build in multiple threads. The test parameters (numITCount) are usually tuned to fully use the test machine, and -T will cause a lot of test failures due to resource exhaustion. (and if you are on a very large machine, you're probably still better off increasing numITCount when running the full test suite)

Since we do not actually test the shaded artifacts, there is not much point in building them in Jenkins, and they are not useful 95% percent of the time during local development. We could shave off 10 minutes from the CI builds by not building the shaded artifacts at all. Yetus sets some default profile/property that we could re-use for this, and of course this could also be used locally.

For local development builds that aren't deployed to a real cluster I usually just use:

mvn clean package -DskipTests -am -pl phoenix-core :
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for Apache Phoenix 5.1.0-SNAPSHOT:
[INFO]
[INFO] Phoenix Hbase 2.1.6 compatibility .................. SUCCESS [ 4.551 s]
[INFO] Apache Phoenix ..................................... SUCCESS [ 1.355 s]
[INFO] Phoenix Core ....................................... SUCCESS [ 27.730 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 34.456 s

lhofhansl · 2020-10-18T19:05:08Z

Perhaps we can have a maven option to not build the -embedded jar.

stoty · 2020-10-19T05:06:53Z

The new patch implements both of your suggestions, @lhofhansl .
Note that this changes the embedded client maven coordinates.
At the same time, the new module structure should make adding new client variants easier.(for PHOENIX-6144, specifically)

lhofhansl · 2020-10-19T07:01:09Z

Awesome!
Did a skim... Looks great. And I like the structure for adding new clients.

I'll take a closer look tomorrow.

lhofhansl

Tried it. Works fine!

lhofhansl · 2020-10-19T16:22:00Z

Wait... Looks like it's still taking a very long time. (The previous run wasn't finished... My mistake)
20 minutes and going. :(

But I see it is doing the building in parallel. So I guess we achieved was we set out to do with this one. Just still slow. :(
I'll stick with my approval. So still +1

lhofhansl · 2020-10-19T16:25:27Z

Sorry for the noise - it's a bit early for me in seems. Now assembly fails for me. I'll come back when I fully tested this.

stoty · 2020-10-19T16:39:21Z

I've rebased the patch, as we had some (unrelated) build breaking bugs in master, that @virajjasani has found and fixed. @lhofhansl.
It builds fine now.

stoty · 2020-10-19T16:39:36Z

(!) A patch to the testing environment has been detected.
Re-executing against the patched versions to perform further tests.
The console is at https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-925/2/console in case of problems.

lhofhansl · 2020-10-19T16:52:19Z

When I do git apply with the patch I see that we still have the old, SLOW, exclude patterns. (in phoenix-client-parent/phoenix-client/pom.xml)
Might be a mistake about how I apply the patch. But let's double check.

stoty · 2020-10-19T16:54:27Z

There are two patches, and github only exports the second, I guess.

simplify shading rules to speed it up make client builds parallelizable make embedded client skippable change embedded client maven coordinates

stoty · 2020-10-19T16:55:56Z

I've squashed them.

lhofhansl · 2020-10-19T16:58:59Z

git am did it. Sigh.

joshelser

Goes from 7.5mins to 4mins for me. Looks like a nice win!

I have not done any validation with the embedded artifact, though.

lhofhansl · 2020-10-19T17:27:45Z

Yeah. Looks good now.

stoty · 2020-10-19T17:56:20Z

Hmm. Looks like Yetus cannot correctly handle PRs that consist of more than one patch, even when working directly from GitHub.

stoty · 2020-10-19T19:25:26Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 29s	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	3m 38s	Maven dependency ordering for branch
+1 💚	mvninstall	33m 36s	master passed
+1 💚	compile	1m 19s	master passed
+1 💚	javadoc	1m 28s	master passed
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 15s	Maven dependency ordering for patch
+1 💚	mvninstall	18m 8s	the patch passed
+1 💚	compile	1m 16s	the patch passed
+1 💚	javac	1m 16s	the patch passed
+1 💚	shellcheck	0m 1s	There were no new shellcheck issues.
+1 💚	shelldocs	0m 1s	There were no new shelldocs issues.
-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 9s	The patch has no ill-formed XML file.
-1 ❌	javadoc	0m 10s	phoenix-assembly in the patch failed.
		_ Other Tests _
-1 ❌	unit	99m 22s	root in the patch failed.
-1 ❌	asflicense	1m 52s	The patch generated 8 ASF License warnings.
		166m 4s

Subsystem	Report/Notes
Docker	ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-925/2/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#925
Optional Tests	dupname asflicense shellcheck shelldocs javac javadoc unit xml compile
uname	Linux d93ab81a4843 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev/phoenix-personality.sh
git revision	master / `5b58d9b`
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-925/2/artifact/yetus-general-check/output/whitespace-eol.txt
javadoc	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-925/2/artifact/yetus-general-check/output/patch-javadoc-phoenix-assembly.txt
unit	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-925/2/artifact/yetus-general-check/output/patch-unit-root.txt
Test Results	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-925/2/testReport/
asflicense	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-925/2/artifact/yetus-general-check/output/patch-asflicense-problems.txt
Max. process+thread count	6701 (vs. ulimit of 30000)
modules	C: phoenix-client-parent/phoenix-client phoenix-assembly phoenix-client-parent/phoenix-client-embedded . phoenix-client phoenix-client-parent U: .
Console output	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-925/2/console
versions	git=2.7.4 maven=3.3.9 shellcheck=0.7.0
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

stoty requested review from lhofhansl, ankitsinghal, chrajeshbabu and joshelser October 17, 2020 09:01

lhofhansl approved these changes Oct 19, 2020

View reviewed changes

lhofhansl self-requested a review October 19, 2020 16:25

stoty force-pushed the PHOENIX-6193 branch from 4e8128f to d6037a1 Compare October 19, 2020 16:38

PHOENIX-6193 PHOENIX-6151 slows down shading

de8d142

simplify shading rules to speed it up make client builds parallelizable make embedded client skippable change embedded client maven coordinates

stoty force-pushed the PHOENIX-6193 branch from d6037a1 to de8d142 Compare October 19, 2020 16:55

joshelser approved these changes Oct 19, 2020

View reviewed changes

stoty closed this Oct 19, 2020

stoty deleted the PHOENIX-6193 branch July 29, 2021 13:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHOENIX-6193 PHOENIX-6151 slows down shading #925

PHOENIX-6193 PHOENIX-6151 slows down shading #925

stoty commented Oct 17, 2020

stoty commented Oct 17, 2020

lhofhansl commented Oct 17, 2020

lhofhansl commented Oct 17, 2020 •

edited

stoty commented Oct 18, 2020

lhofhansl commented Oct 18, 2020

stoty commented Oct 19, 2020

lhofhansl commented Oct 19, 2020

lhofhansl left a comment

lhofhansl commented Oct 19, 2020 •

edited

lhofhansl commented Oct 19, 2020

stoty commented Oct 19, 2020 •

edited

stoty commented Oct 19, 2020

lhofhansl commented Oct 19, 2020 •

edited

stoty commented Oct 19, 2020

stoty commented Oct 19, 2020

lhofhansl commented Oct 19, 2020

joshelser left a comment

lhofhansl commented Oct 19, 2020

stoty commented Oct 19, 2020

stoty commented Oct 19, 2020

PHOENIX-6193 PHOENIX-6151 slows down shading #925

PHOENIX-6193 PHOENIX-6151 slows down shading #925

Conversation

stoty commented Oct 17, 2020

stoty commented Oct 17, 2020

lhofhansl commented Oct 17, 2020

lhofhansl commented Oct 17, 2020 • edited

stoty commented Oct 18, 2020

lhofhansl commented Oct 18, 2020

stoty commented Oct 19, 2020

lhofhansl commented Oct 19, 2020

lhofhansl left a comment

Choose a reason for hiding this comment

lhofhansl commented Oct 19, 2020 • edited

lhofhansl commented Oct 19, 2020

stoty commented Oct 19, 2020 • edited

stoty commented Oct 19, 2020

lhofhansl commented Oct 19, 2020 • edited

stoty commented Oct 19, 2020

stoty commented Oct 19, 2020

lhofhansl commented Oct 19, 2020

joshelser left a comment

Choose a reason for hiding this comment

lhofhansl commented Oct 19, 2020

stoty commented Oct 19, 2020

stoty commented Oct 19, 2020

lhofhansl commented Oct 17, 2020 •

edited

lhofhansl commented Oct 19, 2020 •

edited

stoty commented Oct 19, 2020 •

edited

lhofhansl commented Oct 19, 2020 •

edited