You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by GitBox <gi...@apache.org> on 2020/11/20 01:04:52 UTC
[GitHub] [phoenix] shahrs87 opened a new pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
shahrs87 opened a new pull request #978:
URL: https://github.com/apache/phoenix/pull/978
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #978:
URL: https://github.com/apache/phoenix/pull/978#issuecomment-740140805
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 30s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 12m 31s | master passed |
| +1 :green_heart: | compile | 0m 57s | master passed |
| +1 :green_heart: | checkstyle | 1m 36s | master passed |
| +1 :green_heart: | javadoc | 0m 43s | master passed |
| +0 :ok: | spotbugs | 2m 52s | phoenix-core in master has 966 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 7m 34s | the patch passed |
| +1 :green_heart: | compile | 0m 53s | the patch passed |
| +1 :green_heart: | javac | 0m 53s | the patch passed |
| -1 :x: | checkstyle | 1m 39s | phoenix-core: The patch generated 64 new + 2726 unchanged - 15 fixed = 2790 total (was 2741) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 0m 45s | the patch passed |
| +1 :green_heart: | spotbugs | 3m 0s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 102m 58s | phoenix-core in the patch failed. |
| +1 :green_heart: | asflicense | 0m 28s | The patch does not generate ASF License warnings. |
| | | 139m 10s | |
| 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-978/7/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/978 |
| JIRA Issue | PHOENIX-6213 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux 48c117a8fbe4 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev/phoenix-personality.sh |
| git revision | master / 0106d96 |
| 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-978/7/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-978/7/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-978/7/testReport/ |
| Max. process+thread count | 7127 (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-978/7/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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] shahrs87 commented on pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
shahrs87 commented on pull request #978:
URL: https://github.com/apache/phoenix/pull/978#issuecomment-739561829
> Please also squash the commits with a good commit message to get ready for merging.
I have squashed all commits into one. Please review again. Thank you !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #978:
URL: https://github.com/apache/phoenix/pull/978#issuecomment-733434254
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 33s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 12m 38s | master passed |
| +1 :green_heart: | compile | 1m 0s | master passed |
| +1 :green_heart: | checkstyle | 1m 36s | master passed |
| +1 :green_heart: | javadoc | 0m 45s | master passed |
| +0 :ok: | spotbugs | 2m 53s | phoenix-core in master has 966 extant spotbugs warnings. |
| -0 :warning: | patch | 3m 0s | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 7m 38s | the patch passed |
| +1 :green_heart: | compile | 0m 55s | the patch passed |
| +1 :green_heart: | javac | 0m 55s | the patch passed |
| -1 :x: | checkstyle | 1m 39s | phoenix-core: The patch generated 64 new + 2728 unchanged - 18 fixed = 2792 total (was 2746) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 0m 43s | the patch passed |
| +1 :green_heart: | spotbugs | 3m 7s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 101m 8s | phoenix-core in the patch failed. |
| +1 :green_heart: | asflicense | 0m 27s | The patch does not generate ASF License warnings. |
| | | 137m 44s | |
| 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-978/4/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/978 |
| JIRA Issue | PHOENIX-6213 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux b632b3c959aa 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev/phoenix-personality.sh |
| git revision | master / f1f92aa |
| 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-978/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-978/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-978/4/testReport/ |
| Max. process+thread count | 7074 (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-978/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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] shahrs87 commented on a change in pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
shahrs87 commented on a change in pull request #978:
URL: https://github.com/apache/phoenix/pull/978#discussion_r532782887
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionScanner.java
##########
@@ -479,6 +486,8 @@ void deleteCForQ(Tuple result, List<Cell> results, UngroupedAggregateRegionObser
delete.addColumns(deleteCF, deleteCQ, ts);
// force tephra to ignore this deletes
delete.setAttribute(PhoenixTransactionContext.TX_ROLLBACK_ATTRIBUTE_KEY, new byte[0]);
+ // TODO: should we set the source of operation attribute here.
+ // TODO: This will be true if we drop column or drop table
Review comment:
@gjacoby126 Could you please advise whether should we add the attribute here ? This codepath is used when we drop the column which will in turn generate Delete Marker.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] shahrs87 commented on a change in pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
shahrs87 commented on a change in pull request #978:
URL: https://github.com/apache/phoenix/pull/978#discussion_r531824853
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java
##########
@@ -943,6 +961,163 @@ public void testDeleteFilterWithMultipleIndexes() throws Exception {
}
}
}
-}
+ /*
+ Tests whether we have cell tags in delete marker for
+ ClientSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteClientDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName + " WHERE v1 = 'foo'";
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ // Add tag "customer-delete" to delete marker.
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ClientSelectDeleteMutationPlan
+ verifyDeletePlan(delete, DeleteCompiler.ClientSelectDeleteMutationPlan.class, props);
+ executeDelete(delete, props, 1);
+ String startRowKeyForBaseTable = "1";
+ String startRowKeyForIndexTable = "foo";
+ // Make sure that Delete Marker has cell tag for base table
+ // and has no cell tag for index table.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null);
+ }
+
+ /*
+ Tests whether we have cell tags in delete marker for
+ ServerSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteServerDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName;
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ServerSelectDeleteMutationPlan
+ verifyDeletePlan(delete, DeleteCompiler.ServerSelectDeleteMutationPlan.class, props);
Review comment:
@virajjasani That's correct. This is my understanding also. Cc @gjacoby126
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] shahrs87 commented on a change in pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
shahrs87 commented on a change in pull request #978:
URL: https://github.com/apache/phoenix/pull/978#discussion_r537120671
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionScanner.java
##########
@@ -479,6 +486,10 @@ void deleteCForQ(Tuple result, List<Cell> results, UngroupedAggregateRegionObser
delete.addColumns(deleteCF, deleteCQ, ts);
// force tephra to ignore this deletes
delete.setAttribute(PhoenixTransactionContext.TX_ROLLBACK_ATTRIBUTE_KEY, new byte[0]);
+ // TODO: We need to set SOURCE_OPERATION_ATTRIB here also. The control will come here if
Review comment:
> Good question, @shahrs87 . I think we should set the attribute there.
@gjacoby126 I started doing changes for setting source operation attribute here. When we drop a column, we create delete markers with delete type "DELETE_COLUMN" for the base table and also we delete metadata for that column in SYSCAT table. Is it reasonable to have the feature first for just deleting rows from data table and then we can extend it for drop columns use case. Please advise. Thank you !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] virajjasani commented on a change in pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #978:
URL: https://github.com/apache/phoenix/pull/978#discussion_r532786843
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
##########
@@ -991,6 +999,47 @@ public void preBatchMutateWithExceptions(ObserverContext<RegionCoprocessorEnviro
}
}
+ /**
+ * Set Cell Tags to delete markers with source of operation attribute.
+ * @param miniBatchOp
+ * @throws IOException
+ */
+ private void setDeleteAttributes(MiniBatchOperationInProgress<Mutation> miniBatchOp)
+ throws IOException {
+ for (int i = 0; i < miniBatchOp.size(); i++) {
+ Mutation m = miniBatchOp.getOperation(i);
+ if (!(m instanceof Delete)) {
+ // Ignore if it is not Delete type.
+ continue;
+ }
+ byte[] sourceOpAttr = m.getAttribute(QueryServices.SOURCE_OPERATION_ATTRIB);
+ if (sourceOpAttr == null) {
+ continue;
+ }
+
+ Tag sourceOpTag = new ArrayBackedTag(PhoenixTagType.SOURCE_OPERATION_TAG_TYPE,
+ sourceOpAttr);
+ List<Cell> updatedCells = new ArrayList<>();
+ for (CellScanner cellScanner = m.cellScanner(); cellScanner.advance();) {
+ Cell cell = cellScanner.current();
+ // TODO: Cell#getTagsArray, Cell#getTagsOffset, Cell#getTagsLength are deprecated.
+ //TODO: Need to replace them with new methods.
+ List<Tag> tags = PrivateCellUtil.getTags(cell);
+ tags.add(sourceOpTag);
+ // TODO: PrivateCellUtil's IA is Private. Need to change it to LP with
+ // TODO: IA.COPROC.
+ Cell updatedCell = PrivateCellUtil.createCell(cell, tags);
+ updatedCells.add(updatedCell);
+ }
+ m.getFamilyCellMap().clear();
+ // Clear and add new Cells to the Mutation.
+ for (Cell cell : updatedCells) {
+ Delete d = (Delete) m;
+ d.addDeleteMarker(cell);
+ }
Review comment:
Sounds good, and now with new Builder as IA.LP, we are good to use LP APIs until upgrading to 2.5.x/1.7.x (hopefully) and till then we can use `ArrayBackedTag` directly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #978:
URL: https://github.com/apache/phoenix/pull/978#issuecomment-740209192
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 29s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 12m 46s | master passed |
| +1 :green_heart: | compile | 0m 56s | master passed |
| +1 :green_heart: | checkstyle | 1m 29s | master passed |
| +1 :green_heart: | javadoc | 0m 47s | master passed |
| +0 :ok: | spotbugs | 2m 52s | phoenix-core in master has 966 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 7m 39s | the patch passed |
| +1 :green_heart: | compile | 0m 54s | the patch passed |
| +1 :green_heart: | javac | 0m 54s | the patch passed |
| -1 :x: | checkstyle | 1m 41s | phoenix-core: The patch generated 64 new + 2726 unchanged - 15 fixed = 2790 total (was 2741) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 0m 43s | the patch passed |
| +1 :green_heart: | spotbugs | 3m 0s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 100m 12s | phoenix-core in the patch failed. |
| +1 :green_heart: | asflicense | 0m 27s | The patch does not generate ASF License warnings. |
| | | 136m 42s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | phoenix.end2end.ViewMetadataIT |
| 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-978/8/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/978 |
| JIRA Issue | PHOENIX-6213 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux 0def9ae988ed 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev/phoenix-personality.sh |
| git revision | master / 0106d96 |
| 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-978/8/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-978/8/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-978/8/testReport/ |
| Max. process+thread count | 6749 (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-978/8/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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] shahrs87 commented on a change in pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
shahrs87 commented on a change in pull request #978:
URL: https://github.com/apache/phoenix/pull/978#discussion_r532781817
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
##########
@@ -991,6 +999,47 @@ public void preBatchMutateWithExceptions(ObserverContext<RegionCoprocessorEnviro
}
}
+ /**
+ * Set Cell Tags to delete markers with source of operation attribute.
+ * @param miniBatchOp
+ * @throws IOException
+ */
+ private void setDeleteAttributes(MiniBatchOperationInProgress<Mutation> miniBatchOp)
+ throws IOException {
+ for (int i = 0; i < miniBatchOp.size(); i++) {
+ Mutation m = miniBatchOp.getOperation(i);
+ if (!(m instanceof Delete)) {
+ // Ignore if it is not Delete type.
+ continue;
+ }
+ byte[] sourceOpAttr = m.getAttribute(QueryServices.SOURCE_OPERATION_ATTRIB);
+ if (sourceOpAttr == null) {
+ continue;
+ }
+
+ Tag sourceOpTag = new ArrayBackedTag(PhoenixTagType.SOURCE_OPERATION_TAG_TYPE,
+ sourceOpAttr);
+ List<Cell> updatedCells = new ArrayList<>();
+ for (CellScanner cellScanner = m.cellScanner(); cellScanner.advance();) {
+ Cell cell = cellScanner.current();
+ // TODO: Cell#getTagsArray, Cell#getTagsOffset, Cell#getTagsLength are deprecated.
+ //TODO: Need to replace them with new methods.
+ List<Tag> tags = PrivateCellUtil.getTags(cell);
+ tags.add(sourceOpTag);
+ // TODO: PrivateCellUtil's IA is Private. Need to change it to LP with
+ // TODO: IA.COPROC.
+ Cell updatedCell = PrivateCellUtil.createCell(cell, tags);
+ updatedCells.add(updatedCell);
+ }
+ m.getFamilyCellMap().clear();
+ // Clear and add new Cells to the Mutation.
+ for (Cell cell : updatedCells) {
+ Delete d = (Delete) m;
+ d.addDeleteMarker(cell);
+ }
Review comment:
> but eventually, do you think performing this entire cleanup + adding new delete markers could be done at HBase side using Mutation API?
@virajjasani This jira https://issues.apache.org/jira/browse/PHOENIX-6213 was initially a hbase jira. Also refer to this thread here: https://lists.apache.org/thread.html/r68bd14516383d3f624fc0185fff21ab6044f548c1f31e751e4130915%40%3Cdev.hbase.apache.org%3E where hbase developers advised to add cell tags in phoenix coproc. :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] gjacoby126 commented on a change in pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on a change in pull request #978:
URL: https://github.com/apache/phoenix/pull/978#discussion_r529045025
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java
##########
@@ -943,6 +960,164 @@ public void testDeleteFilterWithMultipleIndexes() throws Exception {
}
}
}
-}
+ /*
+ Tests whether we have cell tags in delete marker for
+ ClientSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteClientDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName + " WHERE v1 = 'foo'";
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ // Add tag "customer-delete" to delete marker.
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ClientSelectDeleteMutationPlan
+ verifyDeletePlan(delete, "ClientSelectDeleteMutationPlan", props);
+ executeDelete(delete, props, 1);
+ String startRowKeyForBaseTable = "1";
+ String startRowKeyForIndexTable = "foo";
+ // Make sure that Delete Marker has cell tag for base table
+ // and has no cell tag for index table.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null);
+ }
+
+ /*
+ Tests whether we have cell tags in delete marker for
+ ServerSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteServerDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName;
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ServerSelectDeleteMutationPlan
+ verifyDeletePlan(delete, "ServerSelectDeleteMutationPlan", props);
Review comment:
Would putting them at package level work?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] gjacoby126 commented on a change in pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on a change in pull request #978:
URL: https://github.com/apache/phoenix/pull/978#discussion_r534405992
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionScanner.java
##########
@@ -479,6 +486,8 @@ void deleteCForQ(Tuple result, List<Cell> results, UngroupedAggregateRegionObser
delete.addColumns(deleteCF, deleteCQ, ts);
// force tephra to ignore this deletes
delete.setAttribute(PhoenixTransactionContext.TX_ROLLBACK_ATTRIBUTE_KEY, new byte[0]);
+ // TODO: should we set the source of operation attribute here.
+ // TODO: This will be true if we drop column or drop table
Review comment:
Good question, @shahrs87 . I think we should set the attribute there.
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
##########
@@ -991,6 +1000,49 @@ public void preBatchMutateWithExceptions(ObserverContext<RegionCoprocessorEnviro
}
}
+ /**
+ * Set Cell Tags to delete markers with source of operation attribute.
+ * @param miniBatchOp
+ * @throws IOException
+ */
+ private void setDeleteAttributes(MiniBatchOperationInProgress<Mutation> miniBatchOp)
+ throws IOException {
+ for (int i = 0; i < miniBatchOp.size(); i++) {
+ Mutation m = miniBatchOp.getOperation(i);
+ if (!(m instanceof Delete)) {
+ // Ignore if it is not Delete type.
+ continue;
+ }
+ byte[] sourceOpAttr = m.getAttribute(QueryServices.SOURCE_OPERATION_ATTRIB);
+ if (sourceOpAttr == null) {
+ continue;
+ }
+
+ Tag sourceOpTag = new ArrayBackedTag(PhoenixTagType.SOURCE_OPERATION_TAG_TYPE,
+ sourceOpAttr);
+ List<Cell> updatedCells = new ArrayList<>();
+ for (CellScanner cellScanner = m.cellScanner(); cellScanner.advance();) {
+ Cell cell = cellScanner.current();
+ RawCell rawCell = (RawCell)cell;
+ List<Tag> tags = new ArrayList<>();
+ Iterator<Tag> tagsIterator = rawCell.getTags();
+ while (tagsIterator.hasNext()) {
+ tags.add(tagsIterator.next());
+ }
+ tags.add(sourceOpTag);
+ // TODO: PrivateCellUtil's IA is Private. Need to change it to LP with IA.COPROC.
Review comment:
Can update the comment with a TODO pointing to the HBase JIRA introducing the new LP(COPROC) API you made.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] gjacoby126 commented on a change in pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on a change in pull request #978:
URL: https://github.com/apache/phoenix/pull/978#discussion_r528022823
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
##########
@@ -991,6 +999,46 @@ public void preBatchMutateWithExceptions(ObserverContext<RegionCoprocessorEnviro
}
}
+ /*
+ Set Cell Tags to delete markers with source of operation attribute.
+ */
+ private void setDeleteAttributes(MiniBatchOperationInProgress<Mutation> miniBatchOp)
+ throws IOException {
+ for (int i = 0; i < miniBatchOp.size(); i++) {
+ Mutation m = miniBatchOp.getOperation(i);
+ if (!(m instanceof Delete)) {
+ // Ignore if it is not Delete type.
+ continue;
+ }
+ byte[] sourceOpAttr = m.getAttribute(QueryServices.SOURCE_OPERATION_ATTRIB);
+ if (sourceOpAttr == null) {
+ continue;
+ }
+
+ // TODO: Which tag implementation to use ? ArrayBackedTag or ByteBufferTag ?
+ Tag sourceOpTag = new ArrayBackedTag(PhoenixTagType.SOURCE_OPERATION_TAG_TYPE, sourceOpAttr);
+ List<Cell> updatedCells = new ArrayList<>();
+ for (CellScanner cellScanner = m.cellScanner(); cellScanner.advance();) {
+ Cell cell = cellScanner.current();
+ // TODO: Cell#getTagsArray, Cell#getTagsOffset, Cell#getTagsLength are deprecated.
Review comment:
See RawCell, which unlike the Cell tag methods, isn't deprecated, and is IA LimitedPrivate.COPROC. You'll probably have to downcast.
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
##########
@@ -991,6 +999,46 @@ public void preBatchMutateWithExceptions(ObserverContext<RegionCoprocessorEnviro
}
}
+ /*
+ Set Cell Tags to delete markers with source of operation attribute.
+ */
+ private void setDeleteAttributes(MiniBatchOperationInProgress<Mutation> miniBatchOp)
+ throws IOException {
+ for (int i = 0; i < miniBatchOp.size(); i++) {
+ Mutation m = miniBatchOp.getOperation(i);
+ if (!(m instanceof Delete)) {
+ // Ignore if it is not Delete type.
+ continue;
+ }
+ byte[] sourceOpAttr = m.getAttribute(QueryServices.SOURCE_OPERATION_ATTRIB);
+ if (sourceOpAttr == null) {
+ continue;
+ }
+
+ // TODO: Which tag implementation to use ? ArrayBackedTag or ByteBufferTag ?
Review comment:
I believe ArrayBackedTag is for Cells built on the heap, and ByteBufferTag is for Cells built off-heap. You're building Cells on-heap, so I assume you'd want ArrayBackedTag. That said, we need to be very careful we're not accidentally leaking memory here by swapping the provided Cells with new ones -- I'm not clear on when the underlying HBase is using on-heap vs off-heap Cells. Needs more research
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
##########
@@ -991,6 +999,46 @@ public void preBatchMutateWithExceptions(ObserverContext<RegionCoprocessorEnviro
}
}
+ /*
+ Set Cell Tags to delete markers with source of operation attribute.
+ */
+ private void setDeleteAttributes(MiniBatchOperationInProgress<Mutation> miniBatchOp)
+ throws IOException {
+ for (int i = 0; i < miniBatchOp.size(); i++) {
+ Mutation m = miniBatchOp.getOperation(i);
+ if (!(m instanceof Delete)) {
+ // Ignore if it is not Delete type.
+ continue;
+ }
+ byte[] sourceOpAttr = m.getAttribute(QueryServices.SOURCE_OPERATION_ATTRIB);
+ if (sourceOpAttr == null) {
+ continue;
+ }
+
+ // TODO: Which tag implementation to use ? ArrayBackedTag or ByteBufferTag ?
+ Tag sourceOpTag = new ArrayBackedTag(PhoenixTagType.SOURCE_OPERATION_TAG_TYPE, sourceOpAttr);
+ List<Cell> updatedCells = new ArrayList<>();
+ for (CellScanner cellScanner = m.cellScanner(); cellScanner.advance();) {
+ Cell cell = cellScanner.current();
+ // TODO: Cell#getTagsArray, Cell#getTagsOffset, Cell#getTagsLength are deprecated.
+ //TODO: Need to replace them with new methods.
+ List<Tag> tags = TagUtil.asList(cell.getTagsArray(), cell.getTagsOffset(),
+ cell.getTagsLength());
+ tags.add(sourceOpTag);
+ // TODO: PrivateCellUtil's IA is Private. Need to change it to LP with
+ // TODO: IA.COPROC.
Review comment:
See RawCellBuilder, which is IA LimitedPrivate.COPROC.
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java
##########
@@ -943,6 +960,164 @@ public void testDeleteFilterWithMultipleIndexes() throws Exception {
}
}
}
-}
+ /*
+ Tests whether we have cell tags in delete marker for
+ ClientSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteClientDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName + " WHERE v1 = 'foo'";
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ // Add tag "customer-delete" to delete marker.
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ClientSelectDeleteMutationPlan
+ verifyDeletePlan(delete, "ClientSelectDeleteMutationPlan", props);
+ executeDelete(delete, props, 1);
+ String startRowKeyForBaseTable = "1";
+ String startRowKeyForIndexTable = "foo";
+ // Make sure that Delete Marker has cell tag for base table
+ // and has no cell tag for index table.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null);
+ }
+
+ /*
+ Tests whether we have cell tags in delete marker for
+ ServerSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteServerDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName;
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ServerSelectDeleteMutationPlan
+ verifyDeletePlan(delete, "ServerSelectDeleteMutationPlan", props);
+ executeDelete(delete, props, 2);
+
+ String startRowKeyForBaseTable = "1";
+ String startRowKeyForIndexTable = "foo";
+ // Make sure that Delete Marker has cell tag for base table
+ // and has no cell tag for index table.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null);
+ }
+
+ /*
+ Tests whether we have cell tags in delete marker for
+ MultiRowDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteMultiRowDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName + " WHERE k = 1";
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+ // Don't create index table. We will use MultiRowDeleteMutationPlan
+ // if there is no index present for a table.
+ createAndUpsertTable(tableName, null, props);
+ // Make sure that the plan creates is of MultiRowDeleteMutationPlan
+ verifyDeletePlan(delete, "MultiRowDeleteMutationPlan", props);
+ executeDelete(delete, props, 1);
+ String startRowKeyForBaseTable = "1";
+ // Make sure that Delete Marker has cell tag for base table.
+ // We haven't created index table for this test case.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ }
+
+ /*
+ Verify whether plan that we create for delete statement is of planName
+ */
+ private void verifyDeletePlan(String delete, String planName, Properties props)
+ throws SQLException {
+ try(Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ conn.setAutoCommit(true);
+ PhoenixStatement stmt = conn.createStatement().unwrap(PhoenixStatement.class);
+ SQLParser parser = new SQLParser(delete);
+ DeleteStatement deleteStmt = (DeleteStatement) parser.parseStatement();
+ DeleteCompiler compiler = new DeleteCompiler(stmt, null);
+ MutationPlan plan = compiler.compile(deleteStmt);
+ assertTrue(plan.getClass().getName().contains(planName));
+ }
+ }
+ private void createAndUpsertTable(String tableName, String indexName, Properties props)
+ throws SQLException {
+ String ddl = "CREATE TABLE " + tableName +
+ " (k INTEGER NOT NULL PRIMARY KEY, v1 VARCHAR, v2 VARCHAR)";
+ try(Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ conn.setAutoCommit(true);
+ try (Statement statement = conn.createStatement()) {
+ statement.execute(ddl);
+ if (indexName != null) {
+ String indexDdl1 = "CREATE INDEX " + indexName + " ON " + tableName + "(v1,v2)";
+ statement.execute(indexDdl1);
+ }
+ }
+ conn.createStatement().execute(
+ "upsert into " + tableName + " values (1, 'foo', 'foo1')");
+ conn.createStatement().execute(
+ "upsert into " + tableName + " values (2, 'bar', 'bar1')");
+ conn.commit();
+ }
+ }
+
+ private void executeDelete(String delete, Properties props, int deleteRowCount)
+ throws SQLException {
+ try(Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ conn.setAutoCommit(true);
+ try (Statement statement = conn.createStatement()) {
+ int rs = statement.executeUpdate(delete);
+ assertEquals( deleteRowCount, rs);
+ }
+ }
+ }
+ /*
+ Verify whether we have tags present for base table and not present for
+ index tables.
+ */
+ private void checkTagPresentInDeleteMarker(String tableName, String startRowKey,
+ boolean tagPresent, String tagValue) throws IOException {
+ List<Cell> values = new ArrayList<>();
+ TableName table = TableName.valueOf(tableName);
+ // Scan table with specified startRowKey
+ for (HRegion region : getUtility().getHBaseCluster().getRegions(table)) {
+ values.clear();
+ Scan scan = new Scan();
+ // Make sure to set rawScan to true so that we will get Delete Markers.
+ scan.setRaw(true);
+ scan.withStartRow(Bytes.toBytes(startRowKey));
+ RegionScanner scanner = region.getScanner(scan);
+ scanner.next(values);
+ if (!values.isEmpty()) {
+ break;
+ }
+ }
+ assertTrue("Values shouldn't be empty", !values.isEmpty());
+ Cell first = values.get(0);
+ assertTrue("First cell should be delete marker ",
+ PrivateCellUtil.isDelete(first.getType().getCode()));
+ List<Tag> tags = PrivateCellUtil.getTags(first);
+ if (tagPresent) {
+ assertEquals(1, tags.size());
+ Optional<Tag> optional =
+ PrivateCellUtil.getTag(first, PhoenixTagType.SOURCE_OPERATION_TAG_TYPE);
Review comment:
PrivateCellUtil is IA.Private -- need to use one of the IA.LimitedPrivate or better classes such as RawCell
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java
##########
@@ -943,6 +960,164 @@ public void testDeleteFilterWithMultipleIndexes() throws Exception {
}
}
}
-}
+ /*
+ Tests whether we have cell tags in delete marker for
+ ClientSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteClientDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName + " WHERE v1 = 'foo'";
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ // Add tag "customer-delete" to delete marker.
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ClientSelectDeleteMutationPlan
+ verifyDeletePlan(delete, "ClientSelectDeleteMutationPlan", props);
+ executeDelete(delete, props, 1);
+ String startRowKeyForBaseTable = "1";
+ String startRowKeyForIndexTable = "foo";
+ // Make sure that Delete Marker has cell tag for base table
+ // and has no cell tag for index table.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null);
+ }
+
+ /*
+ Tests whether we have cell tags in delete marker for
+ ServerSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteServerDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName;
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ServerSelectDeleteMutationPlan
+ verifyDeletePlan(delete, "ServerSelectDeleteMutationPlan", props);
Review comment:
rather than hardcode the string, can use the class name
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java
##########
@@ -943,6 +960,164 @@ public void testDeleteFilterWithMultipleIndexes() throws Exception {
}
}
}
-}
+ /*
+ Tests whether we have cell tags in delete marker for
+ ClientSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteClientDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName + " WHERE v1 = 'foo'";
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ // Add tag "customer-delete" to delete marker.
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ClientSelectDeleteMutationPlan
+ verifyDeletePlan(delete, "ClientSelectDeleteMutationPlan", props);
+ executeDelete(delete, props, 1);
+ String startRowKeyForBaseTable = "1";
+ String startRowKeyForIndexTable = "foo";
+ // Make sure that Delete Marker has cell tag for base table
+ // and has no cell tag for index table.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null);
+ }
+
+ /*
+ Tests whether we have cell tags in delete marker for
+ ServerSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteServerDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName;
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ServerSelectDeleteMutationPlan
+ verifyDeletePlan(delete, "ServerSelectDeleteMutationPlan", props);
+ executeDelete(delete, props, 2);
+
+ String startRowKeyForBaseTable = "1";
+ String startRowKeyForIndexTable = "foo";
+ // Make sure that Delete Marker has cell tag for base table
+ // and has no cell tag for index table.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null);
+ }
+
+ /*
+ Tests whether we have cell tags in delete marker for
+ MultiRowDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteMultiRowDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName + " WHERE k = 1";
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+ // Don't create index table. We will use MultiRowDeleteMutationPlan
+ // if there is no index present for a table.
+ createAndUpsertTable(tableName, null, props);
+ // Make sure that the plan creates is of MultiRowDeleteMutationPlan
+ verifyDeletePlan(delete, "MultiRowDeleteMutationPlan", props);
+ executeDelete(delete, props, 1);
+ String startRowKeyForBaseTable = "1";
+ // Make sure that Delete Marker has cell tag for base table.
+ // We haven't created index table for this test case.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ }
+
+ /*
+ Verify whether plan that we create for delete statement is of planName
+ */
+ private void verifyDeletePlan(String delete, String planName, Properties props)
+ throws SQLException {
+ try(Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ conn.setAutoCommit(true);
+ PhoenixStatement stmt = conn.createStatement().unwrap(PhoenixStatement.class);
+ SQLParser parser = new SQLParser(delete);
+ DeleteStatement deleteStmt = (DeleteStatement) parser.parseStatement();
+ DeleteCompiler compiler = new DeleteCompiler(stmt, null);
+ MutationPlan plan = compiler.compile(deleteStmt);
+ assertTrue(plan.getClass().getName().contains(planName));
+ }
+ }
+ private void createAndUpsertTable(String tableName, String indexName, Properties props)
+ throws SQLException {
+ String ddl = "CREATE TABLE " + tableName +
+ " (k INTEGER NOT NULL PRIMARY KEY, v1 VARCHAR, v2 VARCHAR)";
+ try(Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ conn.setAutoCommit(true);
+ try (Statement statement = conn.createStatement()) {
+ statement.execute(ddl);
+ if (indexName != null) {
+ String indexDdl1 = "CREATE INDEX " + indexName + " ON " + tableName + "(v1,v2)";
+ statement.execute(indexDdl1);
+ }
+ }
+ conn.createStatement().execute(
+ "upsert into " + tableName + " values (1, 'foo', 'foo1')");
+ conn.createStatement().execute(
+ "upsert into " + tableName + " values (2, 'bar', 'bar1')");
+ conn.commit();
+ }
+ }
+
+ private void executeDelete(String delete, Properties props, int deleteRowCount)
+ throws SQLException {
+ try(Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ conn.setAutoCommit(true);
+ try (Statement statement = conn.createStatement()) {
+ int rs = statement.executeUpdate(delete);
+ assertEquals( deleteRowCount, rs);
+ }
+ }
+ }
+ /*
+ Verify whether we have tags present for base table and not present for
+ index tables.
+ */
+ private void checkTagPresentInDeleteMarker(String tableName, String startRowKey,
+ boolean tagPresent, String tagValue) throws IOException {
+ List<Cell> values = new ArrayList<>();
+ TableName table = TableName.valueOf(tableName);
+ // Scan table with specified startRowKey
+ for (HRegion region : getUtility().getHBaseCluster().getRegions(table)) {
+ values.clear();
+ Scan scan = new Scan();
+ // Make sure to set rawScan to true so that we will get Delete Markers.
+ scan.setRaw(true);
+ scan.withStartRow(Bytes.toBytes(startRowKey));
+ RegionScanner scanner = region.getScanner(scan);
+ scanner.next(values);
+ if (!values.isEmpty()) {
+ break;
+ }
+ }
+ assertTrue("Values shouldn't be empty", !values.isEmpty());
+ Cell first = values.get(0);
+ assertTrue("First cell should be delete marker ",
+ PrivateCellUtil.isDelete(first.getType().getCode()));
Review comment:
Can use CellUtil.isDelete, which is IA.Public
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java
##########
@@ -943,6 +960,164 @@ public void testDeleteFilterWithMultipleIndexes() throws Exception {
}
}
}
-}
+ /*
+ Tests whether we have cell tags in delete marker for
+ ClientSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteClientDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName + " WHERE v1 = 'foo'";
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ // Add tag "customer-delete" to delete marker.
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ClientSelectDeleteMutationPlan
+ verifyDeletePlan(delete, "ClientSelectDeleteMutationPlan", props);
Review comment:
Rather than hardcode the string, can use the classname
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] virajjasani commented on a change in pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #978:
URL: https://github.com/apache/phoenix/pull/978#discussion_r531661541
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
##########
@@ -991,6 +999,47 @@ public void preBatchMutateWithExceptions(ObserverContext<RegionCoprocessorEnviro
}
}
+ /**
+ * Set Cell Tags to delete markers with source of operation attribute.
+ * @param miniBatchOp
+ * @throws IOException
+ */
+ private void setDeleteAttributes(MiniBatchOperationInProgress<Mutation> miniBatchOp)
+ throws IOException {
+ for (int i = 0; i < miniBatchOp.size(); i++) {
+ Mutation m = miniBatchOp.getOperation(i);
+ if (!(m instanceof Delete)) {
+ // Ignore if it is not Delete type.
+ continue;
+ }
+ byte[] sourceOpAttr = m.getAttribute(QueryServices.SOURCE_OPERATION_ATTRIB);
+ if (sourceOpAttr == null) {
+ continue;
+ }
+
+ Tag sourceOpTag = new ArrayBackedTag(PhoenixTagType.SOURCE_OPERATION_TAG_TYPE,
+ sourceOpAttr);
+ List<Cell> updatedCells = new ArrayList<>();
+ for (CellScanner cellScanner = m.cellScanner(); cellScanner.advance();) {
+ Cell cell = cellScanner.current();
+ // TODO: Cell#getTagsArray, Cell#getTagsOffset, Cell#getTagsLength are deprecated.
+ //TODO: Need to replace them with new methods.
+ List<Tag> tags = PrivateCellUtil.getTags(cell);
+ tags.add(sourceOpTag);
+ // TODO: PrivateCellUtil's IA is Private. Need to change it to LP with
+ // TODO: IA.COPROC.
+ Cell updatedCell = PrivateCellUtil.createCell(cell, tags);
+ updatedCells.add(updatedCell);
+ }
+ m.getFamilyCellMap().clear();
+ // Clear and add new Cells to the Mutation.
+ for (Cell cell : updatedCells) {
+ Delete d = (Delete) m;
+ d.addDeleteMarker(cell);
+ }
Review comment:
For now I think it's fine to perform this whole operation here, but eventually, do you think performing this entire `cleanup + adding new delete markers` could be done at HBase side using Mutation API? Not very strong point but keeping as much cell mutations to hbase side as possible might be better?
Or maybe keeping this whole batch operation `setDeleteAttributes()` at hbase side as Public/LP API?
WDYT @shahrs87 @gjacoby126 ? Apologies if this was already discussed, I know there have been few nice long discussions overall.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] gjacoby126 commented on a change in pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on a change in pull request #978:
URL: https://github.com/apache/phoenix/pull/978#discussion_r537820280
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionScanner.java
##########
@@ -479,6 +486,10 @@ void deleteCForQ(Tuple result, List<Cell> results, UngroupedAggregateRegionObser
delete.addColumns(deleteCF, deleteCQ, ts);
// force tephra to ignore this deletes
delete.setAttribute(PhoenixTransactionContext.TX_ROLLBACK_ATTRIBUTE_KEY, new byte[0]);
+ // TODO: We need to set SOURCE_OPERATION_ATTRIB here also. The control will come here if
Review comment:
The drop column case can be a separate JIRA.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] shahrs87 commented on pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
shahrs87 commented on pull request #978:
URL: https://github.com/apache/phoenix/pull/978#issuecomment-735947330
@gjacoby126 Other than using Hbase's Private Annotation apis, do you have any other review comment that I can address ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #978:
URL: https://github.com/apache/phoenix/pull/978#issuecomment-735041115
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 31s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 12m 33s | master passed |
| +1 :green_heart: | compile | 0m 58s | master passed |
| +1 :green_heart: | checkstyle | 1m 35s | master passed |
| +1 :green_heart: | javadoc | 0m 44s | master passed |
| +0 :ok: | spotbugs | 2m 54s | phoenix-core in master has 966 extant spotbugs warnings. |
| -0 :warning: | patch | 3m 1s | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 7m 42s | the patch passed |
| +1 :green_heart: | compile | 0m 52s | the patch passed |
| +1 :green_heart: | javac | 0m 52s | the patch passed |
| -1 :x: | checkstyle | 1m 41s | phoenix-core: The patch generated 61 new + 2728 unchanged - 18 fixed = 2789 total (was 2746) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 0m 45s | the patch passed |
| +1 :green_heart: | spotbugs | 3m 4s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 100m 40s | phoenix-core in the patch failed. |
| +1 :green_heart: | asflicense | 0m 27s | The patch does not generate ASF License warnings. |
| | | 137m 5s | |
| 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-978/5/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/978 |
| JIRA Issue | PHOENIX-6213 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux e6eaebb43956 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev/phoenix-personality.sh |
| git revision | master / 993ad5e |
| 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-978/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-978/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-978/5/testReport/ |
| Max. process+thread count | 6954 (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-978/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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] gjacoby126 commented on a change in pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on a change in pull request #978:
URL: https://github.com/apache/phoenix/pull/978#discussion_r529047017
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java
##########
@@ -943,6 +960,164 @@ public void testDeleteFilterWithMultipleIndexes() throws Exception {
}
}
}
-}
+ /*
+ Tests whether we have cell tags in delete marker for
+ ClientSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteClientDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName + " WHERE v1 = 'foo'";
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ // Add tag "customer-delete" to delete marker.
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ClientSelectDeleteMutationPlan
+ verifyDeletePlan(delete, "ClientSelectDeleteMutationPlan", props);
+ executeDelete(delete, props, 1);
+ String startRowKeyForBaseTable = "1";
+ String startRowKeyForIndexTable = "foo";
+ // Make sure that Delete Marker has cell tag for base table
+ // and has no cell tag for index table.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null);
+ }
+
+ /*
+ Tests whether we have cell tags in delete marker for
+ ServerSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteServerDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName;
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ServerSelectDeleteMutationPlan
+ verifyDeletePlan(delete, "ServerSelectDeleteMutationPlan", props);
Review comment:
(If package level wouldn't, I think I'd rather they be public than have the strings hard-coded, so that the dependency is explicit)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] shahrs87 commented on pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
shahrs87 commented on pull request #978:
URL: https://github.com/apache/phoenix/pull/978#issuecomment-731398665
@gjacoby126 While I fix the checkstyle errors, could you please help me review the patch.
I have added TODO comment in couple of places where I am not clear what to do. Could you please advise there. Thank you !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #978:
URL: https://github.com/apache/phoenix/pull/978#issuecomment-731389197
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 33s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| -1 :x: | 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 :green_heart: | mvninstall | 13m 6s | master passed |
| +1 :green_heart: | compile | 1m 1s | master passed |
| +1 :green_heart: | checkstyle | 1m 37s | master passed |
| +1 :green_heart: | javadoc | 0m 43s | master passed |
| +0 :ok: | spotbugs | 2m 54s | phoenix-core in master has 965 extant spotbugs warnings. |
| -0 :warning: | patch | 3m 1s | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 7m 28s | the patch passed |
| +1 :green_heart: | compile | 0m 53s | the patch passed |
| +1 :green_heart: | javac | 0m 53s | the patch passed |
| -1 :x: | checkstyle | 1m 40s | phoenix-core: The patch generated 38 new + 2735 unchanged - 8 fixed = 2773 total (was 2743) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 0m 42s | the patch passed |
| +1 :green_heart: | spotbugs | 3m 9s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 101m 8s | phoenix-core in the patch failed. |
| +1 :green_heart: | asflicense | 0m 27s | The patch does not generate ASF License warnings. |
| | | 138m 0s | |
| 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-978/3/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/978 |
| JIRA Issue | PHOENIX-6213 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux 752a36890f26 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev/phoenix-personality.sh |
| git revision | master / 2af2adf |
| 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-978/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-978/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-978/3/testReport/ |
| Max. process+thread count | 6921 (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-978/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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #978:
URL: https://github.com/apache/phoenix/pull/978#issuecomment-739562126
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 5m 35s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
||| _ master Compile Tests _ |
| -1 :x: | mvninstall | 2m 51s | root in master failed. |
| -1 :x: | compile | 0m 13s | phoenix-core in master failed. |
| -1 :x: | checkstyle | 0m 6s | The patch fails to run checkstyle in phoenix-core |
| -1 :x: | javadoc | 0m 9s | phoenix-core in master failed. |
| -1 :x: | spotbugs | 0m 9s | phoenix-core in master failed. |
||| _ Patch Compile Tests _ |
| -1 :x: | mvninstall | 0m 21s | root in the patch failed. |
| -1 :x: | compile | 0m 8s | phoenix-core in the patch failed. |
| -1 :x: | javac | 0m 8s | phoenix-core in the patch failed. |
| -1 :x: | checkstyle | 0m 6s | The patch fails to run checkstyle in phoenix-core |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| -1 :x: | javadoc | 0m 8s | phoenix-core in the patch failed. |
| -1 :x: | spotbugs | 0m 8s | phoenix-core in the patch failed. |
||| _ Other Tests _ |
| -1 :x: | unit | 0m 8s | phoenix-core in the patch failed. |
| +1 :green_heart: | asflicense | 0m 10s | The patch does not generate ASF License warnings. |
| | | 10m 49s | |
| 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-978/6/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/978 |
| JIRA Issue | PHOENIX-6213 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux 328ac1708eba 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev/phoenix-personality.sh |
| git revision | master / 7d9f78d |
| Default Java | Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08 |
| mvninstall | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-978/6/artifact/yetus-general-check/output/branch-mvninstall-root.txt |
| compile | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-978/6/artifact/yetus-general-check/output/branch-compile-phoenix-core.txt |
| checkstyle | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-978/6/artifact/yetus-general-check/output/buildtool-branch-checkstyle-phoenix-core.txt |
| javadoc | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-978/6/artifact/yetus-general-check/output/branch-javadoc-phoenix-core.txt |
| spotbugs | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-978/6/artifact/yetus-general-check/output/branch-spotbugs-phoenix-core.txt |
| mvninstall | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-978/6/artifact/yetus-general-check/output/patch-mvninstall-root.txt |
| compile | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-978/6/artifact/yetus-general-check/output/patch-compile-phoenix-core.txt |
| javac | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-978/6/artifact/yetus-general-check/output/patch-compile-phoenix-core.txt |
| checkstyle | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-978/6/artifact/yetus-general-check/output/buildtool-patch-checkstyle-phoenix-core.txt |
| javadoc | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-978/6/artifact/yetus-general-check/output/patch-javadoc-phoenix-core.txt |
| spotbugs | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-978/6/artifact/yetus-general-check/output/patch-spotbugs-phoenix-core.txt |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-978/6/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-978/6/testReport/ |
| Max. process+thread count | 62 (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-978/6/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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] shahrs87 commented on a change in pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
shahrs87 commented on a change in pull request #978:
URL: https://github.com/apache/phoenix/pull/978#discussion_r537120770
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
##########
@@ -991,6 +1000,49 @@ public void preBatchMutateWithExceptions(ObserverContext<RegionCoprocessorEnviro
}
}
+ /**
+ * Set Cell Tags to delete markers with source of operation attribute.
+ * @param miniBatchOp
+ * @throws IOException
+ */
+ private void setDeleteAttributes(MiniBatchOperationInProgress<Mutation> miniBatchOp)
+ throws IOException {
+ for (int i = 0; i < miniBatchOp.size(); i++) {
+ Mutation m = miniBatchOp.getOperation(i);
+ if (!(m instanceof Delete)) {
+ // Ignore if it is not Delete type.
+ continue;
+ }
+ byte[] sourceOpAttr = m.getAttribute(QueryServices.SOURCE_OPERATION_ATTRIB);
+ if (sourceOpAttr == null) {
+ continue;
+ }
+
+ Tag sourceOpTag = new ArrayBackedTag(PhoenixTagType.SOURCE_OPERATION_TAG_TYPE,
+ sourceOpAttr);
+ List<Cell> updatedCells = new ArrayList<>();
+ for (CellScanner cellScanner = m.cellScanner(); cellScanner.advance();) {
+ Cell cell = cellScanner.current();
+ RawCell rawCell = (RawCell)cell;
+ List<Tag> tags = new ArrayList<>();
+ Iterator<Tag> tagsIterator = rawCell.getTags();
+ while (tagsIterator.hasNext()) {
+ tags.add(tagsIterator.next());
+ }
+ tags.add(sourceOpTag);
+ // TODO: PrivateCellUtil's IA is Private. Need to change it to LP with IA.COPROC.
Review comment:
Done in latest revision.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] gjacoby126 merged pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
gjacoby126 merged pull request #978:
URL: https://github.com/apache/phoenix/pull/978
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] virajjasani commented on a change in pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #978:
URL: https://github.com/apache/phoenix/pull/978#discussion_r531606951
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java
##########
@@ -943,6 +961,163 @@ public void testDeleteFilterWithMultipleIndexes() throws Exception {
}
}
}
-}
+ /*
+ Tests whether we have cell tags in delete marker for
+ ClientSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteClientDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName + " WHERE v1 = 'foo'";
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ // Add tag "customer-delete" to delete marker.
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ClientSelectDeleteMutationPlan
+ verifyDeletePlan(delete, DeleteCompiler.ClientSelectDeleteMutationPlan.class, props);
+ executeDelete(delete, props, 1);
+ String startRowKeyForBaseTable = "1";
+ String startRowKeyForIndexTable = "foo";
+ // Make sure that Delete Marker has cell tag for base table
+ // and has no cell tag for index table.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null);
+ }
+
+ /*
+ Tests whether we have cell tags in delete marker for
+ ServerSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteServerDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName;
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ServerSelectDeleteMutationPlan
+ verifyDeletePlan(delete, DeleteCompiler.ServerSelectDeleteMutationPlan.class, props);
+ executeDelete(delete, props, 2);
+
+ String startRowKeyForBaseTable = "1";
+ String startRowKeyForIndexTable = "foo";
+ // Make sure that Delete Marker has cell tag for base table
+ // and has no cell tag for index table.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null);
+ }
+
+ /*
+ Tests whether we have cell tags in delete marker for
+ MultiRowDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteMultiRowDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName + " WHERE k = 1";
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+ // Don't create index table. We will use MultiRowDeleteMutationPlan
+ // if there is no index present for a table.
+ createAndUpsertTable(tableName, null, props);
+ // Make sure that the plan creates is of MultiRowDeleteMutationPlan
+ verifyDeletePlan(delete, DeleteCompiler.MultiRowDeleteMutationPlan.class, props);
+ executeDelete(delete, props, 1);
+ String startRowKeyForBaseTable = "1";
+ // Make sure that Delete Marker has cell tag for base table.
+ // We haven't created index table for this test case.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ }
+
+ /*
+ Verify whether plan that we create for delete statement is of planClass
+ */
+ private void verifyDeletePlan(String delete, Class planClass, Properties props)
Review comment:
nit: `Class<? extends MutationPlan> planClass` ?
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java
##########
@@ -943,6 +961,163 @@ public void testDeleteFilterWithMultipleIndexes() throws Exception {
}
}
}
-}
+ /*
+ Tests whether we have cell tags in delete marker for
+ ClientSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteClientDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName + " WHERE v1 = 'foo'";
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ // Add tag "customer-delete" to delete marker.
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ClientSelectDeleteMutationPlan
+ verifyDeletePlan(delete, DeleteCompiler.ClientSelectDeleteMutationPlan.class, props);
+ executeDelete(delete, props, 1);
+ String startRowKeyForBaseTable = "1";
+ String startRowKeyForIndexTable = "foo";
+ // Make sure that Delete Marker has cell tag for base table
+ // and has no cell tag for index table.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null);
+ }
+
+ /*
+ Tests whether we have cell tags in delete marker for
+ ServerSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteServerDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName;
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ServerSelectDeleteMutationPlan
+ verifyDeletePlan(delete, DeleteCompiler.ServerSelectDeleteMutationPlan.class, props);
+ executeDelete(delete, props, 2);
+
+ String startRowKeyForBaseTable = "1";
+ String startRowKeyForIndexTable = "foo";
+ // Make sure that Delete Marker has cell tag for base table
+ // and has no cell tag for index table.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null);
+ }
+
+ /*
+ Tests whether we have cell tags in delete marker for
+ MultiRowDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteMultiRowDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName + " WHERE k = 1";
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+ // Don't create index table. We will use MultiRowDeleteMutationPlan
+ // if there is no index present for a table.
+ createAndUpsertTable(tableName, null, props);
+ // Make sure that the plan creates is of MultiRowDeleteMutationPlan
+ verifyDeletePlan(delete, DeleteCompiler.MultiRowDeleteMutationPlan.class, props);
+ executeDelete(delete, props, 1);
+ String startRowKeyForBaseTable = "1";
+ // Make sure that Delete Marker has cell tag for base table.
+ // We haven't created index table for this test case.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ }
+
+ /*
+ Verify whether plan that we create for delete statement is of planClass
+ */
+ private void verifyDeletePlan(String delete, Class planClass, Properties props)
+ throws SQLException {
+ try(Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ conn.setAutoCommit(true);
+ PhoenixStatement stmt = conn.createStatement().unwrap(PhoenixStatement.class);
+ SQLParser parser = new SQLParser(delete);
+ DeleteStatement deleteStmt = (DeleteStatement) parser.parseStatement();
+ DeleteCompiler compiler = new DeleteCompiler(stmt, null);
+ MutationPlan plan = compiler.compile(deleteStmt);
+ assertEquals(plan.getClass(), planClass);
+ }
+ }
+ private void createAndUpsertTable(String tableName, String indexName, Properties props)
+ throws SQLException {
+ String ddl = "CREATE TABLE " + tableName +
+ " (k INTEGER NOT NULL PRIMARY KEY, v1 VARCHAR, v2 VARCHAR)";
+ try(Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ conn.setAutoCommit(true);
+ try (Statement statement = conn.createStatement()) {
+ statement.execute(ddl);
+ if (indexName != null) {
+ String indexDdl1 = "CREATE INDEX " + indexName + " ON " + tableName + "(v1,v2)";
+ statement.execute(indexDdl1);
+ }
+ }
+ conn.createStatement().execute(
+ "upsert into " + tableName + " values (1, 'foo', 'foo1')");
+ conn.createStatement().execute(
+ "upsert into " + tableName + " values (2, 'bar', 'bar1')");
+ conn.commit();
+ }
+ }
+
+ private void executeDelete(String delete, Properties props, int deleteRowCount)
+ throws SQLException {
+ try(Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ conn.setAutoCommit(true);
+ try (Statement statement = conn.createStatement()) {
+ int rs = statement.executeUpdate(delete);
+ assertEquals( deleteRowCount, rs);
+ }
+ }
+ }
+ /*
+ Verify whether we have tags present for base table and not present for
+ index tables.
+ */
+ private void checkTagPresentInDeleteMarker(String tableName, String startRowKey,
+ boolean tagPresent, String tagValue) throws IOException {
+ List<Cell> values = new ArrayList<>();
+ TableName table = TableName.valueOf(tableName);
+ // Scan table with specified startRowKey
+ for (HRegion region : getUtility().getHBaseCluster().getRegions(table)) {
+ values.clear();
+ Scan scan = new Scan();
+ // Make sure to set rawScan to true so that we will get Delete Markers.
+ scan.setRaw(true);
+ scan.withStartRow(Bytes.toBytes(startRowKey));
+ RegionScanner scanner = region.getScanner(scan);
+ scanner.next(values);
+ if (!values.isEmpty()) {
+ break;
+ }
+ }
+ assertTrue("Values shouldn't be empty", !values.isEmpty());
Review comment:
nit: simplify to `assertFalse("Values shouldn't be empty", values.isEmpty());` ?
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java
##########
@@ -943,6 +961,163 @@ public void testDeleteFilterWithMultipleIndexes() throws Exception {
}
}
}
-}
+ /*
+ Tests whether we have cell tags in delete marker for
+ ClientSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteClientDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName + " WHERE v1 = 'foo'";
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ // Add tag "customer-delete" to delete marker.
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ClientSelectDeleteMutationPlan
+ verifyDeletePlan(delete, DeleteCompiler.ClientSelectDeleteMutationPlan.class, props);
+ executeDelete(delete, props, 1);
+ String startRowKeyForBaseTable = "1";
+ String startRowKeyForIndexTable = "foo";
+ // Make sure that Delete Marker has cell tag for base table
+ // and has no cell tag for index table.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null);
+ }
+
+ /*
+ Tests whether we have cell tags in delete marker for
+ ServerSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteServerDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName;
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ServerSelectDeleteMutationPlan
+ verifyDeletePlan(delete, DeleteCompiler.ServerSelectDeleteMutationPlan.class, props);
+ executeDelete(delete, props, 2);
+
+ String startRowKeyForBaseTable = "1";
+ String startRowKeyForIndexTable = "foo";
+ // Make sure that Delete Marker has cell tag for base table
+ // and has no cell tag for index table.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null);
+ }
+
+ /*
+ Tests whether we have cell tags in delete marker for
+ MultiRowDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteMultiRowDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName + " WHERE k = 1";
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+ // Don't create index table. We will use MultiRowDeleteMutationPlan
+ // if there is no index present for a table.
+ createAndUpsertTable(tableName, null, props);
+ // Make sure that the plan creates is of MultiRowDeleteMutationPlan
+ verifyDeletePlan(delete, DeleteCompiler.MultiRowDeleteMutationPlan.class, props);
+ executeDelete(delete, props, 1);
+ String startRowKeyForBaseTable = "1";
+ // Make sure that Delete Marker has cell tag for base table.
+ // We haven't created index table for this test case.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ }
+
+ /*
+ Verify whether plan that we create for delete statement is of planClass
+ */
+ private void verifyDeletePlan(String delete, Class planClass, Properties props)
+ throws SQLException {
+ try(Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ conn.setAutoCommit(true);
+ PhoenixStatement stmt = conn.createStatement().unwrap(PhoenixStatement.class);
+ SQLParser parser = new SQLParser(delete);
+ DeleteStatement deleteStmt = (DeleteStatement) parser.parseStatement();
+ DeleteCompiler compiler = new DeleteCompiler(stmt, null);
+ MutationPlan plan = compiler.compile(deleteStmt);
+ assertEquals(plan.getClass(), planClass);
+ }
+ }
+ private void createAndUpsertTable(String tableName, String indexName, Properties props)
+ throws SQLException {
+ String ddl = "CREATE TABLE " + tableName +
+ " (k INTEGER NOT NULL PRIMARY KEY, v1 VARCHAR, v2 VARCHAR)";
+ try(Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ conn.setAutoCommit(true);
+ try (Statement statement = conn.createStatement()) {
+ statement.execute(ddl);
+ if (indexName != null) {
+ String indexDdl1 = "CREATE INDEX " + indexName + " ON " + tableName + "(v1,v2)";
+ statement.execute(indexDdl1);
+ }
+ }
+ conn.createStatement().execute(
+ "upsert into " + tableName + " values (1, 'foo', 'foo1')");
+ conn.createStatement().execute(
+ "upsert into " + tableName + " values (2, 'bar', 'bar1')");
Review comment:
nit: both these statements can be included with above `try (Statement statement = conn.createStatement())`
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java
##########
@@ -943,6 +961,163 @@ public void testDeleteFilterWithMultipleIndexes() throws Exception {
}
}
}
-}
+ /*
+ Tests whether we have cell tags in delete marker for
+ ClientSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteClientDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName + " WHERE v1 = 'foo'";
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ // Add tag "customer-delete" to delete marker.
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ClientSelectDeleteMutationPlan
+ verifyDeletePlan(delete, DeleteCompiler.ClientSelectDeleteMutationPlan.class, props);
+ executeDelete(delete, props, 1);
+ String startRowKeyForBaseTable = "1";
+ String startRowKeyForIndexTable = "foo";
+ // Make sure that Delete Marker has cell tag for base table
+ // and has no cell tag for index table.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null);
+ }
+
+ /*
+ Tests whether we have cell tags in delete marker for
+ ServerSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteServerDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName;
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ServerSelectDeleteMutationPlan
+ verifyDeletePlan(delete, DeleteCompiler.ServerSelectDeleteMutationPlan.class, props);
Review comment:
`ServerSelectDeleteMutationPlan` is because DELETE query has no WHERE clause and we have index on base table which will be part of queryPlan?
```
boolean runOnServer = isAutoCommit && !hasPreOrPostProcessing && !table.isTransactional() && !hasClientSideIndexes && allowServerMutations;
runOnServer &= queryPlans.get(0).getTableRef().getTable().getType() != PTableType.INDEX;
```
Is this the correct reference?
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java
##########
@@ -943,6 +961,163 @@ public void testDeleteFilterWithMultipleIndexes() throws Exception {
}
}
}
-}
+ /*
+ Tests whether we have cell tags in delete marker for
+ ClientSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteClientDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName + " WHERE v1 = 'foo'";
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ // Add tag "customer-delete" to delete marker.
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ClientSelectDeleteMutationPlan
+ verifyDeletePlan(delete, DeleteCompiler.ClientSelectDeleteMutationPlan.class, props);
+ executeDelete(delete, props, 1);
+ String startRowKeyForBaseTable = "1";
+ String startRowKeyForIndexTable = "foo";
+ // Make sure that Delete Marker has cell tag for base table
+ // and has no cell tag for index table.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null);
+ }
+
+ /*
+ Tests whether we have cell tags in delete marker for
+ ServerSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteServerDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName;
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ServerSelectDeleteMutationPlan
+ verifyDeletePlan(delete, DeleteCompiler.ServerSelectDeleteMutationPlan.class, props);
+ executeDelete(delete, props, 2);
+
+ String startRowKeyForBaseTable = "1";
+ String startRowKeyForIndexTable = "foo";
+ // Make sure that Delete Marker has cell tag for base table
+ // and has no cell tag for index table.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null);
+ }
+
+ /*
+ Tests whether we have cell tags in delete marker for
+ MultiRowDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteMultiRowDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName + " WHERE k = 1";
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+ // Don't create index table. We will use MultiRowDeleteMutationPlan
+ // if there is no index present for a table.
+ createAndUpsertTable(tableName, null, props);
+ // Make sure that the plan creates is of MultiRowDeleteMutationPlan
+ verifyDeletePlan(delete, DeleteCompiler.MultiRowDeleteMutationPlan.class, props);
+ executeDelete(delete, props, 1);
+ String startRowKeyForBaseTable = "1";
+ // Make sure that Delete Marker has cell tag for base table.
+ // We haven't created index table for this test case.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ }
+
+ /*
+ Verify whether plan that we create for delete statement is of planClass
+ */
+ private void verifyDeletePlan(String delete, Class planClass, Properties props)
+ throws SQLException {
+ try(Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ conn.setAutoCommit(true);
+ PhoenixStatement stmt = conn.createStatement().unwrap(PhoenixStatement.class);
+ SQLParser parser = new SQLParser(delete);
+ DeleteStatement deleteStmt = (DeleteStatement) parser.parseStatement();
+ DeleteCompiler compiler = new DeleteCompiler(stmt, null);
+ MutationPlan plan = compiler.compile(deleteStmt);
+ assertEquals(plan.getClass(), planClass);
+ }
+ }
+ private void createAndUpsertTable(String tableName, String indexName, Properties props)
+ throws SQLException {
+ String ddl = "CREATE TABLE " + tableName +
+ " (k INTEGER NOT NULL PRIMARY KEY, v1 VARCHAR, v2 VARCHAR)";
+ try(Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ conn.setAutoCommit(true);
+ try (Statement statement = conn.createStatement()) {
+ statement.execute(ddl);
+ if (indexName != null) {
+ String indexDdl1 = "CREATE INDEX " + indexName + " ON " + tableName + "(v1,v2)";
+ statement.execute(indexDdl1);
+ }
+ }
+ conn.createStatement().execute(
+ "upsert into " + tableName + " values (1, 'foo', 'foo1')");
+ conn.createStatement().execute(
+ "upsert into " + tableName + " values (2, 'bar', 'bar1')");
+ conn.commit();
+ }
+ }
+
+ private void executeDelete(String delete, Properties props, int deleteRowCount)
+ throws SQLException {
+ try(Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ conn.setAutoCommit(true);
+ try (Statement statement = conn.createStatement()) {
+ int rs = statement.executeUpdate(delete);
+ assertEquals( deleteRowCount, rs);
+ }
+ }
+ }
+ /*
+ Verify whether we have tags present for base table and not present for
+ index tables.
+ */
+ private void checkTagPresentInDeleteMarker(String tableName, String startRowKey,
+ boolean tagPresent, String tagValue) throws IOException {
+ List<Cell> values = new ArrayList<>();
+ TableName table = TableName.valueOf(tableName);
+ // Scan table with specified startRowKey
+ for (HRegion region : getUtility().getHBaseCluster().getRegions(table)) {
+ values.clear();
Review comment:
nit: cleaning up entries might not be needed. If `values` has any record, we will be out of this loop.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] shahrs87 commented on a change in pull request #978: [PHOENIX-6213] Extend Cell Tags to Delete object to store source of operation.
Posted by GitBox <gi...@apache.org>.
shahrs87 commented on a change in pull request #978:
URL: https://github.com/apache/phoenix/pull/978#discussion_r528888617
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java
##########
@@ -943,6 +960,164 @@ public void testDeleteFilterWithMultipleIndexes() throws Exception {
}
}
}
-}
+ /*
+ Tests whether we have cell tags in delete marker for
+ ClientSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteClientDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName + " WHERE v1 = 'foo'";
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ // Add tag "customer-delete" to delete marker.
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ClientSelectDeleteMutationPlan
+ verifyDeletePlan(delete, "ClientSelectDeleteMutationPlan", props);
Review comment:
DeleteCompiler#ClientSelectDeleteMutationPlan and other mutation plan classes are private nested classes and are not visible to test classes. Instead of making them public thought to hardcode the string. Let me know if you want me to make them public. Thank you !
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java
##########
@@ -943,6 +960,164 @@ public void testDeleteFilterWithMultipleIndexes() throws Exception {
}
}
}
-}
+ /*
+ Tests whether we have cell tags in delete marker for
+ ClientSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteClientDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName + " WHERE v1 = 'foo'";
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ // Add tag "customer-delete" to delete marker.
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ClientSelectDeleteMutationPlan
+ verifyDeletePlan(delete, "ClientSelectDeleteMutationPlan", props);
+ executeDelete(delete, props, 1);
+ String startRowKeyForBaseTable = "1";
+ String startRowKeyForIndexTable = "foo";
+ // Make sure that Delete Marker has cell tag for base table
+ // and has no cell tag for index table.
+ checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue);
+ checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null);
+ }
+
+ /*
+ Tests whether we have cell tags in delete marker for
+ ServerSelectDeleteMutationPlan.
+ */
+ @Test
+ public void testDeleteServerDeleteMutationPlan() throws Exception {
+ String tableName = generateUniqueName();
+ String indexName = generateUniqueName();
+ String tagValue = "customer-delete";
+ String delete = "DELETE FROM " + tableName;
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue);
+
+ createAndUpsertTable(tableName, indexName, props);
+ // Make sure that the plan creates is of ServerSelectDeleteMutationPlan
+ verifyDeletePlan(delete, "ServerSelectDeleteMutationPlan", props);
Review comment:
DeleteCompiler#ClientSelectDeleteMutationPlan and other mutation plan classes are private nested classes and are not visible to test classes. Instead of making them public thought to hardcode the string. Let me know if you want me to make them public. Thank you !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org