Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHOENIX-6218 Rows deleted count is incorrect for immutable tables with indexes #961

Merged
merged 3 commits into from
Nov 13, 2020

Conversation

tkhurana
Copy link
Contributor

MutationState.join() ignores index rows in row count so totalRowCount calculated
will reflect the data table always and the index table only if the bestPlan
uses the index table. So ignore the other index tables when determining
the number of tables updated.

…h indexes

MutationState.join() ignores index rows in row count so totalRowCount calculated
will reflect the data table always and the index table only if the bestPlan
uses the index table. So ignore the other index tables when determining
the number of tables updated.
@tkhurana
Copy link
Contributor Author

@kadirozde @dbwong

@dbwong
Copy link
Contributor

dbwong commented Nov 10, 2020

A few questions for this.

  • This is only on the client side delete path changes, this seemed to be trying to handle projected tables but I don't see any test cases with such projections.
  • ClientSelectDeleteMutationPlan has some special case handling for transactional tables, local indexes, and immutable tables. I didn't see test cases with transactional cases and local indexes.

@dbwong
Copy link
Contributor

dbwong commented Nov 10, 2020

Another question, I see the JIRA does not make a difference between sever and client side deletions. Did we test both paths? I'm assuming the new ITs hit the client path but I don't see any assertion of that.

@dbwong
Copy link
Contributor

dbwong commented Nov 10, 2020

Last questions what is the impact to changing the output of MutationState::getUpdateCount() in the paths where that is used? Its not clear to me, I tried to follow the change but only went to the writing of the tuple in one of the iterators...

@tkhurana tkhurana closed this Nov 11, 2020
@tkhurana tkhurana reopened this Nov 11, 2020
@tkhurana
Copy link
Contributor Author

tkhurana commented Nov 11, 2020

Another question, I see the JIRA does not make a difference between sever and client side deletions. Did we test both paths? I'm assuming the new ITs hit the client path but I don't see any assertion of that.

The bug is only for client side deletes. I updated the jira title to reflect that

@tkhurana
Copy link
Contributor Author

Last questions what is the impact to changing the output of MutationState::getUpdateCount() in the paths where that is used? Its not clear to me, I tried to follow the change but only went to the writing of the tuple in one of the iterators...

@dbwong are you suggesting to change MutationState to report row count for all updated tables ?

@tkhurana
Copy link
Contributor Author

tkhurana commented Nov 11, 2020

A few questions for this.

  • This is only on the client side delete path changes, this seemed to be trying to handle projected tables but I don't see any test cases with such projections.
  • ClientSelectDeleteMutationPlan has some special case handling for transactional tables, local indexes, and immutable tables. I didn't see test cases with transactional cases and local indexes.

The tests for immutable tables is parameterized on transactions and local indexes. In the new tests that I added I have skipped the transactions but I can remove that check.

@dbwong I didn't understand what you meant by handling projected tables. Can you please elaborate ?

@stoty
Copy link
Contributor

stoty commented Nov 11, 2020

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 1m 41s 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.
_ 4.x Compile Tests _
+1 💚 mvninstall 12m 29s 4.x passed
+1 💚 compile 1m 1s 4.x passed
+1 💚 checkstyle 0m 36s 4.x passed
+1 💚 javadoc 0m 47s 4.x passed
+0 🆗 spotbugs 3m 10s phoenix-core in 4.x has 946 extant spotbugs warnings.
_ Patch Compile Tests _
+1 💚 mvninstall 6m 15s the patch passed
+1 💚 compile 1m 0s the patch passed
+1 💚 javac 1m 0s the patch passed
-1 ❌ checkstyle 0m 38s phoenix-core: The patch generated 4 new + 364 unchanged - 4 fixed = 368 total (was 368)
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 javadoc 0m 48s the patch passed
+1 💚 spotbugs 3m 19s the patch passed
_ Other Tests _
-1 ❌ unit 170m 6s phoenix-core in the patch failed.
+1 💚 asflicense 0m 29s The patch does not generate ASF License warnings.
204m 48s
Reason Tests
Failed junit tests phoenix.end2end.IndexExtendedIT
phoenix.end2end.UpgradeIT
phoenix.end2end.OrderByWithSpillingIT
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-961/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR #961
Optional Tests dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile
uname Linux c2429db1b997 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 4.x / 21e729f
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-961/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-961/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-961/1/testReport/
Max. process+thread count 6024 (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-961/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
Copy link
Contributor

stoty commented Nov 11, 2020

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 1m 11s 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.
_ 4.x Compile Tests _
+1 💚 mvninstall 11m 44s 4.x passed
+1 💚 compile 1m 0s 4.x passed
+1 💚 checkstyle 0m 37s 4.x passed
+1 💚 javadoc 0m 47s 4.x passed
+0 🆗 spotbugs 3m 7s phoenix-core in 4.x has 946 extant spotbugs warnings.
_ Patch Compile Tests _
+1 💚 mvninstall 6m 17s the patch passed
+1 💚 compile 1m 0s the patch passed
+1 💚 javac 1m 0s the patch passed
-1 ❌ checkstyle 0m 37s phoenix-core: The patch generated 2 new + 366 unchanged - 2 fixed = 368 total (was 368)
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 javadoc 0m 46s the patch passed
+1 💚 spotbugs 3m 21s the patch passed
_ Other Tests _
-1 ❌ unit 247m 15s phoenix-core in the patch failed.
+1 💚 asflicense 0m 58s The patch does not generate ASF License warnings.
282m 4s
Reason Tests
Failed junit tests phoenix.end2end.index.PartialIndexRebuilderIT
phoenix.end2end.IntArithmeticIT
phoenix.end2end.IndexBuildTimestampIT
phoenix.end2end.ViewMetadataIT
TEST-[RangeScanIT_2]
phoenix.end2end.RowValueConstructorOffsetIT
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-961/2/artifact/yetus-general-check/output/Dockerfile
GITHUB PR #961
Optional Tests dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile
uname Linux 2afc6cede537 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 4.x / 21e729f
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-961/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-961/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-961/2/testReport/
Max. process+thread count 6177 (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-961/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.

Copy link
Contributor

@kadirozde kadirozde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, Thanks

This way we don't have to fix the inflated row count by dividing the
number of tables updated.
Works for DELETE queries with and without LIMIT filter.
@stoty
Copy link
Contributor

stoty commented Nov 12, 2020

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 1m 44s 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.
_ 4.x Compile Tests _
+1 💚 mvninstall 12m 27s 4.x passed
+1 💚 compile 1m 16s 4.x passed
+1 💚 checkstyle 0m 44s 4.x passed
+1 💚 javadoc 0m 56s 4.x passed
+0 🆗 spotbugs 4m 12s phoenix-core in 4.x has 946 extant spotbugs warnings.
_ Patch Compile Tests _
+1 💚 mvninstall 8m 2s the patch passed
+1 💚 compile 1m 12s the patch passed
+1 💚 javac 1m 12s the patch passed
-1 ❌ checkstyle 0m 43s phoenix-core: The patch generated 11 new + 355 unchanged - 13 fixed = 366 total (was 368)
+1 💚 whitespace 0m 1s The patch has no whitespace issues.
+1 💚 javadoc 1m 0s the patch passed
+1 💚 spotbugs 4m 17s the patch passed
_ Other Tests _
-1 ❌ unit 203m 14s phoenix-core in the patch failed.
+1 💚 asflicense 0m 32s The patch does not generate ASF License warnings.
243m 14s
Reason Tests
Failed junit tests phoenix.end2end.index.PartialIndexRebuilderIT
phoenix.end2end.index.GlobalIndexOptimizationIT
phoenix.schema.stats.NoOpStatsCollectorIT
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-961/3/artifact/yetus-general-check/output/Dockerfile
GITHUB PR #961
Optional Tests dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile
uname Linux 83fd0bb0a25d 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 4.x / 565b0ea
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-961/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-961/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-961/3/testReport/
Max. process+thread count 5987 (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-961/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.

Copy link
Contributor

@gjacoby126 gjacoby126 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change itself looks good, but can we reduce the amount of duplicate setup code in the tests?

@stoty
Copy link
Contributor

stoty commented Nov 13, 2020

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 1m 9s 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.
_ 4.x Compile Tests _
+1 💚 mvninstall 11m 35s 4.x passed
+1 💚 compile 0m 57s 4.x passed
+1 💚 checkstyle 0m 36s 4.x passed
+1 💚 javadoc 0m 48s 4.x passed
+0 🆗 spotbugs 3m 9s phoenix-core in 4.x has 946 extant spotbugs warnings.
_ Patch Compile Tests _
+1 💚 mvninstall 6m 20s the patch passed
+1 💚 compile 1m 1s the patch passed
+1 💚 javac 1m 1s the patch passed
-1 ❌ checkstyle 0m 37s phoenix-core: The patch generated 11 new + 355 unchanged - 13 fixed = 366 total (was 368)
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 javadoc 0m 46s the patch passed
+1 💚 spotbugs 3m 22s the patch passed
_ Other Tests _
-1 ❌ unit 175m 30s phoenix-core in the patch failed.
+1 💚 asflicense 0m 29s The patch does not generate ASF License warnings.
208m 50s
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-961/4/artifact/yetus-general-check/output/Dockerfile
GITHUB PR #961
Optional Tests dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile
uname Linux 89cb2ba60dec 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 4.x / 565b0ea
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-961/4/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-961/4/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-961/4/testReport/
Max. process+thread count 6186 (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-961/4/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
Copy link
Contributor

stoty commented Nov 13, 2020

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 1m 10s 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.
_ 4.x Compile Tests _
+1 💚 mvninstall 11m 52s 4.x passed
+1 💚 compile 1m 0s 4.x passed
+1 💚 checkstyle 0m 36s 4.x passed
+1 💚 javadoc 0m 45s 4.x passed
+0 🆗 spotbugs 3m 10s phoenix-core in 4.x has 946 extant spotbugs warnings.
_ Patch Compile Tests _
+1 💚 mvninstall 6m 7s the patch passed
+1 💚 compile 0m 57s the patch passed
+1 💚 javac 0m 57s the patch passed
-1 ❌ checkstyle 0m 36s phoenix-core: The patch generated 9 new + 357 unchanged - 11 fixed = 366 total (was 368)
+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 179m 43s phoenix-core in the patch failed.
+1 💚 asflicense 0m 32s The patch does not generate ASF License warnings.
213m 13s
Reason Tests
Failed junit tests phoenix.end2end.index.PartialIndexRebuilderIT
phoenix.end2end.UpsertSelectIT
phoenix.end2end.IndexBuildTimestampIT
phoenix.end2end.AlterTableIT
phoenix.end2end.DropIndexedColsIT
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-961/5/artifact/yetus-general-check/output/Dockerfile
GITHUB PR #961
Optional Tests dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile
uname Linux e7b363394f40 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 4.x / 565b0ea
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-961/5/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-961/5/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-961/5/testReport/
Max. process+thread count 6089 (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-961/5/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
Copy link
Contributor

@tkhurana - 4 of the 5 test failures are just timeouts, but could you please verify that UpsertSelectIT passes locally for you? That failure's coming from a failed assertion. Thanks!

@tkhurana
Copy link
Contributor Author

@tkhurana - 4 of the 5 test failures are just timeouts, but could you please verify that UpsertSelectIT passes locally for you? That failure's coming from a failed assertion. Thanks!

@gjacoby126 Yes, it is passing locally.

Copy link
Contributor

@gjacoby126 gjacoby126 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, thanks @tkhurana

@gjacoby126 gjacoby126 merged commit b97696b into apache:4.x Nov 13, 2020
@gjacoby126
Copy link
Contributor

@tkhurana - I merged into 4.x, could you please also make a version for master?

swaroopak pushed a commit that referenced this pull request Dec 14, 2020
…h indexes (#961)

* PHOENIX-6218 Rows deleted count is incorrect for immutable tables with indexes

Signed-off-by: Swaroopa Kadam <s.kadam@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants