PHOENIX-6295 : Fix non-static inner classes for better memory utilization by virajjasani · Pull Request #1053 · apache/phoenix

virajjasani · 2021-01-04T12:08:48Z

No description provided.

stoty · 2021-01-04T14:45:29Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	4m 20s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 1s	No case conflicting files found.
+1 💚	hbaseanti	0m 0s	Patch does not have any anti-patterns.
+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	14m 24s	master passed
+1 💚	compile	0m 55s	master passed
+1 💚	checkstyle	3m 27s	master passed
+1 💚	javadoc	0m 46s	master passed
+0 🆗	spotbugs	2m 55s	phoenix-core in master has 973 extant spotbugs warnings.
		_ Patch Compile Tests _
+1 💚	mvninstall	7m 29s	the patch passed
+1 💚	compile	0m 54s	the patch passed
+1 💚	javac	0m 54s	the patch passed
-1 ❌	checkstyle	3m 30s	phoenix-core: The patch generated 69 new + 7132 unchanged - 73 fixed = 7201 total (was 7205)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	javadoc	0m 44s	the patch passed
-1 ❌	spotbugs	3m 9s	phoenix-core generated 1 new + 957 unchanged - 16 fixed = 958 total (was 973)
		_ Other Tests _
-1 ❌	unit	110m 14s	phoenix-core in the patch failed.
+1 💚	asflicense	0m 29s	The patch does not generate ASF License warnings.
		156m 11s

Reason	Tests
FindBugs	module:phoenix-core
	org.apache.phoenix.hbase.index.covered.update.IndexUpdateManager$MutationComparator implements Comparator but not Serializable At IndexUpdateManager.java:Serializable At IndexUpdateManager.java:[lines 47-94]

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-1053/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#1053
Optional Tests	dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile
uname	Linux 8334d017343d 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 / `4d5449f`
Default Java	Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08
checkstyle	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1053/1/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt
spotbugs	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1053/1/artifact/yetus-general-check/output/new-spotbugs-phoenix-core.html
unit	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1053/1/artifact/yetus-general-check/output/patch-unit-phoenix-core.txt
Test Results	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1053/1/testReport/
Max. process+thread count	6686 (vs. ulimit of 30000)
modules	C: phoenix-core U: phoenix-core
Console output	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1053/1/console
versions	git=2.7.4 maven=3.3.9 spotbugs=4.1.3
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

gjacoby126

+1

stoty · 2021-01-04T16:52:33Z

Pushed new version to fix RAT confusion

virajjasani · 2021-01-05T06:27:59Z

Although findbugs has valid point that we should let MutationComparator implement Serializable because TreeMaps are serializable, but our treeMap that uses this comparator doesn't seem to be getting (de)serialized in any flow and hence we can ignore this findbug warning? Thoughts @gjacoby126 @stoty ?

stoty · 2021-01-05T06:42:45Z

As we don't seem to use the java serialization framework anywhere, and your patch doesn't change anything WRT the java serialization situation, It doesn't really matter.
You may as well make the class Serializable, as it has no state, and it wouldn't make any difference to us, and silence spotbugs, but it's good either way.

virajjasani · 2021-01-05T06:51:04Z

Yeah sounds better, having the class implement Serializable is not going to harm anyways.

…tion

virajjasani · 2021-01-05T12:48:49Z

Weird, recent builds for both PRs master and 4.x did not succeed to post build status on github:

Could not update commit status, please check if your scan credentials belong to a member of the organization or a collaborator of the repository and repo:status scope is selected
GitHub has been notified of this commit’s build result

@stoty seems like similar message to what we used to get on PR 945

virajjasani · 2021-01-05T12:52:24Z

My bad, I think it's different, build did not procceed beyond mvn install which is failing with:

error waiting for container: unexpected EOF

stoty · 2021-01-05T12:55:03Z

We also had multibranch failures today, there seems to a problem or maintanence on the Jenkins infra.

stoty · 2021-01-05T15:09:57Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 29s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	hbaseanti	0m 0s	Patch does not have any anti-patterns.
+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	14m 16s	master passed
+1 💚	compile	0m 55s	master passed
+1 💚	checkstyle	3m 16s	master passed
+1 💚	javadoc	0m 45s	master passed
+0 🆗	spotbugs	2m 55s	phoenix-core in master has 973 extant spotbugs warnings.
		_ Patch Compile Tests _
+1 💚	mvninstall	7m 29s	the patch passed
+1 💚	compile	0m 55s	the patch passed
+1 💚	javac	0m 55s	the patch passed
-1 ❌	checkstyle	3m 30s	phoenix-core: The patch generated 22 new + 7179 unchanged - 26 fixed = 7201 total (was 7205)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	javadoc	0m 43s	the patch passed
+1 💚	spotbugs	3m 0s	phoenix-core generated 0 new + 957 unchanged - 16 fixed = 957 total (was 973)
		_ Other Tests _
-1 ❌	unit	107m 0s	phoenix-core in the patch failed.
+1 💚	asflicense	0m 28s	The patch does not generate ASF License warnings.
		148m 31s

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-1053/3/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#1053
Optional Tests	dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile
uname	Linux 8442a2837389 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 / `0bac803`
Default Java	Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08
checkstyle	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1053/3/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt
unit	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1053/3/artifact/yetus-general-check/output/patch-unit-phoenix-core.txt
Test Results	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1053/3/testReport/
Max. process+thread count	6521 (vs. ulimit of 30000)
modules	C: phoenix-core U: phoenix-core
Console output	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1053/3/console
versions	git=2.7.4 maven=3.3.9 spotbugs=4.1.3
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

virajjasani · 2021-01-07T05:49:52Z

@gjacoby126 @stoty Could you please help merge this PR and 4.x backport #1054 (and 4.16 backport as 4.x and 4.16 are exactly same as of today. If required, I can raise 4.16 PR )?
After approval, the only change done is to let MutationComparator implement Serializable(to silence spotbug warning we saw earlier).
Thanks

virajjasani · 2021-01-08T05:55:03Z

Thanks for merging this @yanxinyi . Could you please also backport 4.x commit to 4.16 as I was expecting to include this in 4.16. Let me know if you want me to create PR, I can keep it ready.
Thanks

virajjasani mentioned this pull request Jan 4, 2021
PHOENIX-6295 : Fix non-static inner classes for better memory utilization #1054
Merged

gjacoby126 approved these changes Jan 4, 2021
View reviewed changes

PHOENIX-6295: Fix non-static inner classes for better memory utiliza…
ff1b202
…tion

yanxinyi merged commit ca4f784 into apache:masterJan 7, 2021

Navigation Menu

Uh oh!

PHOENIX-6295 : Fix non-static inner classes for better memory utilization - #1053

PHOENIX-6295 : Fix non-static inner classes for better memory utilization#1053
yanxinyi merged 1 commit into
apache:masterfrom
virajjasani:PHOENIX-6295-master

virajjasani commented Jan 4, 2021

Uh oh!

stoty commented Jan 4, 2021

Uh oh!

gjacoby126 left a comment

Uh oh!

stoty commented Jan 4, 2021

Uh oh!

virajjasani commented Jan 5, 2021

Uh oh!

stoty commented Jan 5, 2021

Uh oh!

virajjasani commented Jan 5, 2021

Uh oh!

virajjasani commented Jan 5, 2021•
edited
Loading

Uh oh!

virajjasani commented Jan 5, 2021

Uh oh!

stoty commented Jan 5, 2021

Uh oh!

stoty commented Jan 5, 2021

Uh oh!

virajjasani commented Jan 7, 2021•
edited
Loading

Uh oh!

virajjasani commented Jan 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

virajjasani commented Jan 4, 2021

Uh oh!

stoty commented Jan 4, 2021

Uh oh!

gjacoby126 left a comment

Choose a reason for hiding this comment

Uh oh!

stoty commented Jan 4, 2021

Uh oh!

virajjasani commented Jan 5, 2021

Uh oh!

stoty commented Jan 5, 2021

Uh oh!

virajjasani commented Jan 5, 2021

Uh oh!

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

Uh oh!

Uh oh!

virajjasani commented Jan 5, 2021

Uh oh!

stoty commented Jan 5, 2021

Uh oh!

stoty commented Jan 5, 2021

Uh oh!

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

Uh oh!

Uh oh!

virajjasani commented Jan 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

virajjasani commented Jan 5, 2021•
edited
Loading

virajjasani commented Jan 7, 2021•
edited
Loading