PHOENIX-6420 Wrong result when conditional and regular upserts are passed in the same commit batch by tkhurana · Pull Request #1183 · apache/phoenix

tkhurana · 2021-03-25T05:05:39Z

No description provided.

…ssed in the same commit batch

tkhurana · 2021-03-25T05:22:59Z

@kadirozde @gokceni @gjacoby126 @jpisaac

stoty · 2021-03-25T09:40:08Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 32s	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 appears to include 1 new or modified test files.
		_ 4.x Compile Tests _
+1 💚	mvninstall	17m 20s	4.x passed
+1 💚	compile	1m 44s	4.x passed
+1 💚	checkstyle	1m 15s	4.x passed
+1 💚	javadoc	1m 24s	4.x passed
+0 🆗	spotbugs	5m 18s	phoenix-core in 4.x has 941 extant spotbugs warnings.
		_ Patch Compile Tests _
+1 💚	mvninstall	10m 6s	the patch passed
+1 💚	compile	1m 49s	the patch passed
+1 💚	javac	1m 49s	the patch passed
-1 ❌	checkstyle	1m 18s	phoenix-core: The patch generated 123 new + 803 unchanged - 63 fixed = 926 total (was 866)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	javadoc	1m 21s	the patch passed
+1 💚	spotbugs	5m 51s	phoenix-core generated 0 new + 940 unchanged - 1 fixed = 940 total (was 941)
		_ Other Tests _
+1 💚	unit	223m 26s	phoenix-core in the patch passed.
+1 💚	asflicense	0m 9s	The patch does not generate ASF License warnings.
		273m 52s

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-1183/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#1183
Optional Tests	dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile
uname	Linux 63d6d7375042 4.15.0-128-generic #131-Ubuntu SMP Wed Dec 9 06:57:35 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev/phoenix-personality.sh
git revision	4.x / `ee4ce9f`
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-1183/1/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt
Test Results	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1183/1/testReport/
Max. process+thread count	5010 (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-1183/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.

gokceni · 2021-03-25T15:49:57Z

+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s VALUES('a',1,1) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 2, 1);


Should not counter1 value be 3? We are upserting 1 for counter1 and counter1=1+2

I think it's correct as counter1 in the row is 0 from line number 627 or did I confuse myself 💭

@gokceni @swaroopak if the key already exists, we ignore the values provided in upsert and evaluate the expression on the stored value. So in this the increment happens on '0' which was the previously stored value.

Cool, that's what I meant with the value from line 627.

gokceni · 2021-03-25T15:53:57Z

+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s (pk, counter2) VALUES ('a',125)", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 11, 125);


Recommend adding a check that index row is updated as well

Also how about adding a check for char type column updates?

There is no index defined yet. This change addresses the problem on the data table.

I saw createIndex( on line 624 of this function, that is why I was checking.

That is actually local index

gokceni · 2021-03-25T16:03:17Z

+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s VALUES('a',1,1) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 9, 4);


To see these switched could be nice like row 635 and 634 swapped so that we are sure they are in the same batch and the order doesn't matter in the batch.

gokceni · 2021-03-25T16:06:48Z

- } else {
+ boolean incrementRowCount = dstMutations == this.mutationsMap;
+ // we only need to check if the new mutation batch (srcRows) conflicts with the
+ // last mutation batch


why? could you add it to the comment

Because we only try to merge the new batch with the last one. That is why we only check for conflicts with the last one. All previous batches are not updated.

stoty · 2021-03-26T11:22:49Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 17s	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 appears to include 1 new or modified test files.
		_ 4.x Compile Tests _
+1 💚	mvninstall	15m 2s	4.x passed
+1 💚	compile	1m 9s	4.x passed
+1 💚	checkstyle	0m 43s	4.x passed
+1 💚	javadoc	0m 56s	4.x passed
+0 🆗	spotbugs	3m 56s	phoenix-core in 4.x has 941 extant spotbugs warnings.
		_ Patch Compile Tests _
+1 💚	mvninstall	8m 8s	the patch passed
+1 💚	compile	1m 8s	the patch passed
+1 💚	javac	1m 8s	the patch passed
-1 ❌	checkstyle	0m 55s	phoenix-core: The patch generated 123 new + 803 unchanged - 63 fixed = 926 total (was 866)
+1 💚	whitespace	0m 1s	The patch has no whitespace issues.
+1 💚	javadoc	1m 0s	the patch passed
+1 💚	spotbugs	4m 6s	phoenix-core generated 0 new + 940 unchanged - 1 fixed = 940 total (was 941)
		_ Other Tests _
+1 💚	unit	264m 56s	phoenix-core in the patch passed.
+1 💚	asflicense	0m 11s	The patch does not generate ASF License warnings.
		304m 27s

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-1183/2/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#1183
Optional Tests	dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile
uname	Linux fb0938c38a17 4.15.0-128-generic #131-Ubuntu SMP Wed Dec 9 06:57:35 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev/phoenix-personality.sh
git revision	4.x / `3535708`
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-1183/2/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt
Test Results	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1183/2/testReport/
Max. process+thread count	4784 (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-1183/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.

gokceni

LGTM

swaroopak

Non-binding +1

gjacoby126 · 2021-03-25T22:53:46Z

-import java.util.List;
-import java.util.Properties;
-
+import com.google.common.collect.ImmutableList;


good to use the shaded version of guava for easier porting to 5.x

@gjacoby126 I opened this PR against apache 4.x. The version on master is already using the shaded version.

gjacoby126 · 2021-03-25T23:00:15Z

+ assertEquals(2, state.getNumRows());
+
+ int actualPairs = 0;
+ Iterator<Pair<byte[], List<Mutation>>> mutations = state.toMutations();


A comment here on what you're checking would be helpful.

gjacoby126 · 2021-03-25T23:16:14Z

 if (multiRowMutationState != null) {
 final List<Mutation> deleteMutations = Lists.newArrayList();
- generateMutations(key, mutationTimestamp, serverTimestamp, multiRowMutationState, deleteMutations, null);
+ // for index table there will only be 1 mutation batch in the list


why only one mutation batch? What if there are conflicting batches for the index?

@gjacoby126 For indexes, the only time we see an entry in the map is in case of deletes and that too for immutable indexes. So I don't expect a conflicting update unless someone explicitly does a regular upsert and conditional upsert directly on index table which is theoretically possible but highly unlikely.

gjacoby126

+1, thanks @tkhurana

stoty · 2021-04-05T20:47:17Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	5m 33s	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 appears to include 1 new or modified test files.
		_ 4.x Compile Tests _
+1 💚	mvninstall	15m 13s	4.x passed
+1 💚	compile	1m 3s	4.x passed
+1 💚	checkstyle	0m 40s	4.x passed
+1 💚	javadoc	0m 52s	4.x passed
+0 🆗	spotbugs	3m 13s	phoenix-core in 4.x has 941 extant spotbugs warnings.
		_ Patch Compile Tests _
+1 💚	mvninstall	6m 44s	the patch passed
+1 💚	compile	1m 3s	the patch passed
+1 💚	javac	1m 3s	the patch passed
-1 ❌	checkstyle	0m 43s	phoenix-core: The patch generated 123 new + 803 unchanged - 63 fixed = 926 total (was 866)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	javadoc	0m 47s	the patch passed
+1 💚	spotbugs	3m 27s	phoenix-core generated 0 new + 940 unchanged - 1 fixed = 940 total (was 941)
		_ Other Tests _
-1 ❌	unit	193m 2s	phoenix-core in the patch failed.
+1 💚	asflicense	0m 36s	The patch does not generate ASF License warnings.
		235m 40s

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

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-1183/3/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#1183
Optional Tests	dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile
uname	Linux 4aee66818618 4.15.0-128-generic #131-Ubuntu SMP Wed Dec 9 06:57:35 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev/phoenix-personality.sh
git revision	4.x / `5c57004`
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-1183/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-1183/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-1183/3/testReport/
Max. process+thread count	5304 (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-1183/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.

* PHOENIX-6420 Wrong result when conditional and regular upserts are passed in the same commit batch (apache#1183) * PHOENIX-6420 Wrong result when conditional and regular upserts are passed in the same commit batch * Addressed feedback * Added comments, rename variable * PHOENIX-6387 Conditional updates on tables with indexes (apache#1215) * PHOENIX-6387 Conditional updates on tables with indexes client side * PHOENIX-6387 Conditional updates on tables with indexes server side * Remove the extra read For regular upserts on tables with local index * Addressed review comments

PHOENIX-6420 Wrong result when conditional and regular upserts are pa…
22c8d7e
…ssed in the same commit batch

gokceni suggested changes Mar 25, 2021
View reviewed changes

Addressed feedback
ccd6139

gokceni approved these changes Mar 26, 2021
View reviewed changes

swaroopak approved these changes Mar 26, 2021
View reviewed changes

gjacoby126 reviewed Mar 26, 2021
View reviewed changes

Added comments, rename variable
be7fab8

tkhurana requested a review from gjacoby126 April 5, 2021 16:51

gjacoby126 approved these changes Apr 5, 2021
View reviewed changes

swaroopak merged commit d962363 into apache:4.xApr 6, 2021

Uh oh!

Conversation

tkhurana commented Mar 25, 2021

Uh oh!

tkhurana commented Mar 25, 2021

Uh oh!

stoty commented Mar 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swaroopakMar 25, 2021• 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!

tkhuranaMar 25, 2021• 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stoty commented Mar 26, 2021

Uh oh!

gokceni left a comment

Choose a reason for hiding this comment

Uh oh!

swaroopak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gjacoby126 left a comment

Choose a reason for hiding this comment

Uh oh!

stoty commented Apr 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

swaroopakMar 25, 2021•
edited
Loading

tkhuranaMar 25, 2021•
edited
Loading