PHOENIX-6309 : Use maven enforcer plugin to ban imports from illegal packages (as per project guidelines) by virajjasani · Pull Request #1082 · apache/phoenix

virajjasani · 2021-01-10T15:06:35Z

No description provided.

stoty · 2021-01-11T03:40:06Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 5s	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	15m 42s	master passed
+1 💚	compile	1m 32s	master passed
+1 💚	javadoc	1m 17s	master passed
		_ Patch Compile Tests _
+1 💚	mvninstall	9m 28s	the patch passed
+1 💚	compile	1m 32s	the patch passed
+1 💚	javac	1m 32s	the patch passed
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	xml	0m 2s	The patch has no ill-formed XML file.
+1 💚	javadoc	1m 18s	the patch passed
		_ Other Tests _
-1 ❌	unit	177m 54s	root in the patch failed.
+1 💚	asflicense	0m 27s	The patch does not generate ASF License warnings.
		212m 35s

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-1082/2/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#1082
Optional Tests	dupname asflicense javac javadoc unit xml compile
uname	Linux 381bf3490809 4.15.0-126-generic #129-Ubuntu SMP Mon Nov 23 18:53:38 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev/phoenix-personality.sh
git revision	master / `fe0b3ca`
Default Java	Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08
unit	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1082/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-1082/2/testReport/
Max. process+thread count	5755 (vs. ulimit of 30000)
modules	C: . U: .
Console output	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1082/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.

yanxinyi · 2021-01-11T06:33:26Z

LGTM +1 for the master branch. However, do we have a plan to do the same thing for the 4.x? I remember that we don't have enforcement for certain important. For example, we have import com.google.common.collect.ImmutableMap; and import org.apache.phoenix.thirdparty.com.google.common.collect.ImmutableMap;. Do we have a plan to clean the code and do the same thing at the 4.x branch?

virajjasani · 2021-01-11T06:41:04Z

Thanks @yanxinyi for the review.

Do we have a plan to clean the code and do the same thing at the 4.x branch?

@stoty has already created PHOENIX-6234 for replacing guava with thirdparty in 4.x.
Until then, the only enforcement we can see is to restrict old logging framework: #1083

stoty · 2021-01-11T06:42:22Z

On master, this is somewhat redundant, as dependency-check will fail the build in these cases.
That was never backported to 4.x (but eventually should be)

However, as dependency-check runs AFTER the tests, and the tests still fail more oftent than not, this may be useful if this runs before the tests, (and on 4.x.)

The commons-logging and commons-lang stuff definitely applies to 4.x

virajjasani · 2021-01-11T06:43:39Z

this may be useful if this runs before the tests, (and on 4.x.)

Yes, it does catch this before running tests.

stoty · 2021-01-11T06:44:00Z

Does 4.x still have commons-lang ? That is probably a regression, and you may want to fix it in #1083 and add the rule.

virajjasani · 2021-01-11T06:46:18Z

4.x has many references to commons-lang. Let me fix them and update #1083 quickly.

stoty · 2021-01-11T06:47:57Z

In that it probably not a regression, I just never got around to backporting that cleanup to 4.x

I'll leave it to your discretion wheter you want to that for this ticket.

virajjasani · 2021-01-11T06:49:44Z

Alright, Thanks @stoty @yanxinyi . Better not change it for now in 4.x as it is this way for quite some time (more than 2 yr).
We can revisit this after 4.16 release. Sounds good?

virajjasani · 2021-01-11T06:52:21Z

On master, commons-lang was upgraded as part of PHOENIX-4880

stoty · 2021-01-11T07:00:57Z

Sounds good. (good to know it wasn't me who skipped the 4.x backport)

stoty

The latest version for the rule seems to be 1.0.1
Is there any reason to use an older version?

virajjasani · 2021-01-11T07:07:04Z

My bad, in fact we can use 1.1.0 instead.

virajjasani · 2021-01-11T07:08:46Z

While copying from other project, I somehow missed looking at latest versions. Let me update PR.

virajjasani · 2021-01-11T07:28:30Z

Attaching sample build failure:

[WARNING] Rule 2: de.skuzzle.enforcer.restrictimports.rule.RestrictImports failed with message:
Banned imports detected in TEST code:
Reason: Use commons lang 3
in file: org/apache/phoenix/end2end/AlterTableWithViewsIT.java
org.apache.commons.lang.StringUtils (Line: 41, Matched by: org.apache.commons.lang.**)
Analysis took 0 seconds
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO] [INFO] Apache Phoenix 5.1.0-SNAPSHOT ...................... SUCCESS [ 1.701 s]
[INFO] Phoenix Hbase 2.4.0 compatibility .................. SUCCESS [ 3.718 s]
[INFO] Phoenix Hbase 2.3.0 compatibility .................. SUCCESS [ 0.946 s]
[INFO] Phoenix Hbase 2.2.1 compatibility .................. SUCCESS [ 0.893 s]
[INFO] Phoenix Hbase 2.1.6 compatibility .................. SUCCESS [ 0.620 s]
[INFO] Phoenix Core ....................................... FAILURE [ 3.595 s]
[INFO] Phoenix - Pherf .................................... SKIPPED
[INFO] Phoenix Client Parent .............................. SKIPPED
[INFO] Phoenix Client ..................................... SKIPPED
[INFO] Phoenix Server ..................................... SKIPPED
[INFO] Phoenix - Tracing Web Application .................. SKIPPED
[INFO] Phoenix Assembly ................................... SKIPPED
[INFO] phoenix-tools ...................................... SKIPPED
[INFO] Phoenix Client Embedded 5.1.0-SNAPSHOT ............. SKIPPED
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 12.127 s
[INFO] Finished at: 2021-01-11T12:57:06+05:30
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-enforcer-plugin:3.0.0-M3:enforce (banned-illegal-imports) on project phoenix-core: Some Enforcer rules have failed. Look above for specific messages explaining why the rule failed. -> [Help 1]
[ERROR]

…packages (as per project guidelines)

stoty · 2021-01-11T12:36:02Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 9s	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 _
+1 💚	mvninstall	16m 7s	master passed
+1 💚	compile	1m 31s	master passed
+1 💚	javadoc	1m 17s	master passed
		_ Patch Compile Tests _
+1 💚	mvninstall	9m 17s	the patch passed
+1 💚	compile	1m 36s	the patch passed
+1 💚	javac	1m 36s	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	1m 20s	the patch passed
		_ Other Tests _
-1 ❌	unit	269m 1s	root in the patch failed.
+1 💚	asflicense	0m 30s	The patch does not generate ASF License warnings.
		304m 9s

Reason	Tests
Failed junit tests	phoenix.end2end.index.GlobalMutableNonTxIndexIT

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-1082/3/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#1082
Optional Tests	dupname asflicense javac javadoc unit xml compile
uname	Linux fd5ecc1bcde0 4.15.0-126-generic #129-Ubuntu SMP Mon Nov 23 18:53:38 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev/phoenix-personality.sh
git revision	master / `fe0b3ca`
Default Java	Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08
unit	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1082/3/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-1082/3/testReport/
Max. process+thread count	5740 (vs. ulimit of 30000)
modules	C: . U: .
Console output	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1082/3/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.

stoty approved these changes Jan 11, 2021
View reviewed changes

stoty suggested changes Jan 11, 2021
View reviewed changes

PHOENIX-6309: Use maven enforcer plugin to ban imports from illegal …
d7832fb
…packages (as per project guidelines)

stoty approved these changes Jan 11, 2021
View reviewed changes

stoty closed this Jan 11, 2021

virajjasani deleted the PHOENIX-6309-master branch January 11, 2021 14:45

Uh oh!

Conversation

virajjasani commented Jan 10, 2021

Uh oh!

stoty commented Jan 11, 2021

Uh oh!

yanxinyi commented Jan 11, 2021

Uh oh!

virajjasani commented Jan 11, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stoty commented Jan 11, 2021

Uh oh!

virajjasani commented Jan 11, 2021

Uh oh!

stoty commented Jan 11, 2021

Uh oh!

virajjasani commented Jan 11, 2021

Uh oh!

stoty commented Jan 11, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

virajjasani commented Jan 11, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

virajjasani commented Jan 11, 2021

Uh oh!

stoty commented Jan 11, 2021

Uh oh!

stoty left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

virajjasani commented Jan 11, 2021

Uh oh!

virajjasani commented Jan 11, 2021

Uh oh!

virajjasani commented Jan 11, 2021

Uh oh!

stoty commented Jan 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

virajjasani commented Jan 11, 2021•
edited
Loading

stoty commented Jan 11, 2021•
edited
Loading

virajjasani commented Jan 11, 2021•
edited
Loading

stoty left a comment •
edited
Loading