PHOENIX-6082 : Avoid checkAndPut when altering properties for a table or view with column-encoding enabled by virajjasani · Pull Request #982 · apache/phoenix

virajjasani · 2020-11-22T13:30:40Z

No description provided.

stoty · 2020-11-22T16:50:53Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 18s	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 29s	master passed
+1 💚	compile	0m 59s	master passed
+1 💚	checkstyle	1m 16s	master passed
+1 💚	javadoc	0m 45s	master passed
+0 🆗	spotbugs	3m 8s	phoenix-core in master has 966 extant spotbugs warnings.
		_ Patch Compile Tests _
+1 💚	mvninstall	9m 19s	the patch passed
+1 💚	compile	0m 57s	the patch passed
+1 💚	javac	0m 57s	the patch passed
-1 ❌	checkstyle	1m 15s	phoenix-core: The patch generated 1 new + 2796 unchanged - 4 fixed = 2797 total (was 2800)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	javadoc	0m 46s	the patch passed
+1 💚	spotbugs	3m 20s	the patch passed
		_ Other Tests _
-1 ❌	unit	159m 18s	phoenix-core in the patch failed.
+1 💚	asflicense	0m 21s	The patch does not generate ASF License warnings.
		199m 45s

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-982/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#982
Optional Tests	dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile
uname	Linux de18c8f4b1fe 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 / `f1a0860`
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-982/1/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-982/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-982/1/testReport/
Max. process+thread count	6167 (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-982/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.

stoty · 2020-11-22T21:23:45Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 8s	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 20s	master passed
+1 💚	compile	1m 2s	master passed
+1 💚	checkstyle	1m 16s	master passed
+1 💚	javadoc	0m 48s	master passed
+0 🆗	spotbugs	3m 7s	phoenix-core in master has 966 extant spotbugs warnings.
		_ Patch Compile Tests _
+1 💚	mvninstall	9m 2s	the patch passed
+1 💚	compile	0m 56s	the patch passed
+1 💚	javac	0m 56s	the patch passed
-1 ❌	checkstyle	1m 15s	phoenix-core: The patch generated 1 new + 2796 unchanged - 4 fixed = 2797 total (was 2800)
+1 💚	whitespace	0m 1s	The patch has no whitespace issues.
+1 💚	javadoc	0m 46s	the patch passed
+1 💚	spotbugs	3m 21s	the patch passed
		_ Other Tests _
-1 ❌	unit	158m 10s	phoenix-core in the patch failed.
+1 💚	asflicense	0m 22s	The patch does not generate ASF License warnings.
		198m 2s

Reason	Tests
Failed junit tests	phoenix.end2end.ViewTTLIT

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-982/2/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#982
Optional Tests	dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile
uname	Linux 82fbfb032567 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 / `f1a0860`
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-982/2/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-982/2/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-982/2/testReport/
Max. process+thread count	6216 (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-982/2/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 · 2020-11-23T06:36:37Z

Test failure with ViewTTLIT.testDeleteFromMultipleGlobalIndexes() is not relevant, confirmed locally.

virajjasani · 2020-11-23T13:34:57Z

@ChinmaySKulkarni @yanxinyi could you please take a look?
Thanks

ChinmaySKulkarni

@virajjasani thanks for the patch! I have a few nit-type comments on your tests, otherwise lgtm. Can you please confirm this patch locally by scanning SYSTEM.MUTEX via HBase shell?

ChinmaySKulkarni · 2020-11-25T18:57:36Z

+ ResultSet resultSet = conn.createStatement().executeQuery(
+ "SELECT * FROM " + PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME);
+ if (resultSet.next()) {
+ isSysMutexEmpty.getAndSet(false);


this can be a simple set() since you're not using the returned value right?

ChinmaySKulkarni · 2020-11-25T19:03:45Z

+ while (!Thread.interrupted() && !conn.isClosed()) {
+ try {
+ ResultSet resultSet = conn.createStatement().executeQuery(
+ "SELECT * FROM " + PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME);


In case we do end up going this route then, just to be safe, can we query the exact expected row in SYSTEM.MUTEX instead of this query? If this gets run in parallel with any of the upgrade tests (where we acquire an upgrade mutex), your test will fail.

… or view with column-encoding enabled

virajjasani

Thanks @ChinmaySKulkarni for the review.

stoty · 2020-11-26T00:05:48Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 6s	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	12m 33s	master passed
+1 💚	compile	0m 59s	master passed
+1 💚	checkstyle	1m 37s	master passed
+1 💚	javadoc	0m 44s	master passed
+0 🆗	spotbugs	2m 55s	phoenix-core in master has 966 extant spotbugs warnings.
		_ Patch Compile Tests _
+1 💚	mvninstall	7m 37s	the patch passed
+1 💚	compile	0m 50s	the patch passed
+1 💚	javac	0m 50s	the patch passed
-1 ❌	checkstyle	1m 41s	phoenix-core: The patch generated 1 new + 2796 unchanged - 4 fixed = 2797 total (was 2800)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	javadoc	0m 42s	the patch passed
+1 💚	spotbugs	3m 3s	the patch passed
		_ Other Tests _
-1 ❌	unit	188m 29s	phoenix-core in the patch failed.
+1 💚	asflicense	0m 27s	The patch does not generate ASF License warnings.
		225m 25s

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-982/3/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#982
Optional Tests	dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile
uname	Linux a59c248be7c6 4.15.0-65-generic #74-Ubuntu SMP Tue Sep 17 17:06:04 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev/phoenix-personality.sh
git revision	master / `457a67c`
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-982/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-982/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-982/3/testReport/
Max. process+thread count	6649 (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-982/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 · 2020-11-26T16:58:50Z

Can you please confirm this patch locally by scanning SYSTEM.MUTEX via HBase shell?

Sure, confirmed by scanning SYSTEM.MUTEX on local cluster with and without this patch.

Initially, updated SYSTEM.MUTEX with KEEP_DELETED_CELLS => 'TRUE' because while executing ALTER TABLE query, Mutex cell is going to be deleted soon (finally block), hence it appears only momentarily.

With patch, no records were found while scanning SYSTEM.MUTEX:

Without patch, executed ALTER TABLE query twice and found two deleted rows (first scan result after first execution of ALTER query, and second scan result after second execution of ALTER query) :

ALTER TABLE query used for testing:

ChinmaySKulkarni

+1 can you please put up a pr against 4.x too?
-- edit: nevermind, just saw #983
Anyone else want to take a look @yanxinyi @gjacoby126 @jpisaac ?

virajjasani · 2020-12-07T07:00:08Z

Thanks for the reviews @ChinmaySKulkarni @yanxinyi

virajjasani mentioned this pull request Nov 22, 2020
PHOENIX-6082 : Avoid checkAndPut when altering properties for a table or view with column-encoding enabled #983
Closed

ChinmaySKulkarni reviewed Nov 25, 2020
View reviewed changes

PHOENIX-6082: Avoid checkAndPut when altering properties for a table…
1c23e69
… or view with column-encoding enabled

virajjasani commented Nov 25, 2020
View reviewed changes

Comment threadphoenix-core/src/it/java/org/apache/phoenix/rpc/UpdateCacheIT.java
Comment threadphoenix-core/src/it/java/org/apache/phoenix/rpc/UpdateCacheIT.java
Comment threadphoenix-core/src/it/java/org/apache/phoenix/rpc/UpdateCacheIT.java

ChinmaySKulkarni approved these changes Dec 2, 2020
View reviewed changes

virajjasani closed this Dec 7, 2020

virajjasani deleted the PHOENIX-6082-master branch December 7, 2020 07:00

Uh oh!

Conversation

virajjasani commented Nov 22, 2020

Uh oh!

stoty commented Nov 22, 2020

Uh oh!

stoty commented Nov 22, 2020

Uh oh!

virajjasani commented Nov 23, 2020

Uh oh!

virajjasani commented Nov 23, 2020

Uh oh!

ChinmaySKulkarni left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ChinmaySKulkarniNov 25, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ChinmaySKulkarniNov 25, 2020

Choose a reason for hiding this comment

Uh oh!

virajjasani 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!

Uh oh!

Uh oh!

Uh oh!

stoty commented Nov 26, 2020

Uh oh!

virajjasani commented Nov 26, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChinmaySKulkarni 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 Dec 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

virajjasani left a comment •
edited
Loading

virajjasani commented Nov 26, 2020•
edited
Loading

ChinmaySKulkarni left a comment •
edited
Loading