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/10/23 17:15:09 UTC
[GitHub] [phoenix] tkhurana opened a new pull request #937: PHOENIX-6198 Add option to IndexTool to specify the source table for scan
tkhurana opened a new pull request #937:
URL: https://github.com/apache/phoenix/pull/937
Part of PHOENIX-6182
New option `-fi` or `--from-index` added. When specified, use the index table as the source for verification. Inly works with `-vBEFORE` and `-vONLY` options.
----------------------------------------------------------------
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] tkhurana commented on pull request #937: PHOENIX-6198 Add option to IndexTool to specify the source table for scan
Posted by GitBox <gi...@apache.org>.
tkhurana commented on pull request #937:
URL: https://github.com/apache/phoenix/pull/937#issuecomment-718440351
> Having an IT that test that index table is used would be great (extra tables in index table, run IndexTool without this option see that it doesn't return anything, with this option and observe the failure)
I have comprehensive IT tests for end to end functionality in an upcoming PR. This PR is just for client side changes. I have broken up the feature into multiple smaller PRs to ease the review. Ultimately, the feature branch apache:4.x-PHOENIX-5182 will be merged into mainline.
----------------------------------------------------------------
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] priyankporwal commented on a change in pull request #937: PHOENIX-6198 Add option to IndexTool to specify the source table for scan
Posted by GitBox <gi...@apache.org>.
priyankporwal commented on a change in pull request #937:
URL: https://github.com/apache/phoenix/pull/937#discussion_r514594621
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/mapreduce/util/PhoenixConfigurationUtil.java
##########
@@ -660,6 +663,19 @@ public static String getIndexToolIndexTableName(Configuration configuration) {
return configuration.get(INDEX_TOOL_INDEX_TABLE_NAME);
}
+ public static void setIndexToolSourceTable(Configuration configuration,
+ IndexScrutinyTool.SourceTable sourceTable) {
+ Preconditions.checkNotNull(configuration);
+ Preconditions.checkNotNull(sourceTable);
+ configuration.set(INDEX_TOOL_SOURCE_TABLE, sourceTable.name());
+ }
+
+ public static IndexScrutinyTool.SourceTable getIndexToolSourceTable(Configuration configuration) {
+ Preconditions.checkNotNull(configuration);
+ return IndexScrutinyTool.SourceTable.valueOf(configuration.get(INDEX_TOOL_SOURCE_TABLE,
+ IndexScrutinyTool.SourceTable.DATA_TABLE_SOURCE.name()));
Review comment:
IndexScrutinyTool.SourceTable.DATA_TABLE_SOURCE.name() the default value?
----------------------------------------------------------------
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 #937: PHOENIX-6198 Add option to IndexTool to specify the source table for scan
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #937:
URL: https://github.com/apache/phoenix/pull/937#issuecomment-718592832
: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 2 new or modified test files. |
||| _ 4.x-PHOENIX-5182 Compile Tests _ |
| +1 :green_heart: | mvninstall | 10m 59s | 4.x-PHOENIX-5182 passed |
| +1 :green_heart: | compile | 0m 55s | 4.x-PHOENIX-5182 passed |
| +1 :green_heart: | checkstyle | 0m 53s | 4.x-PHOENIX-5182 passed |
| +1 :green_heart: | javadoc | 0m 46s | 4.x-PHOENIX-5182 passed |
| +0 :ok: | spotbugs | 2m 52s | phoenix-core in 4.x-PHOENIX-5182 has 954 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 5m 22s | the patch passed |
| +1 :green_heart: | compile | 0m 55s | the patch passed |
| +1 :green_heart: | javac | 0m 55s | the patch passed |
| -1 :x: | checkstyle | 0m 55s | phoenix-core: The patch generated 43 new + 975 unchanged - 3 fixed = 1018 total (was 978) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 0m 41s | the patch passed |
| +1 :green_heart: | spotbugs | 3m 3s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 124m 8s | phoenix-core in the patch failed. |
| +1 :green_heart: | asflicense | 0m 36s | The patch does not generate ASF License warnings. |
| | | 155m 19s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | phoenix.end2end.QueryIT |
| | phoenix.end2end.index.ImmutableIndexExtendedIT |
| | phoenix.end2end.NullIT |
| | phoenix.end2end.PointInTimeQueryIT |
| | phoenix.end2end.TenantSpecificViewIndexSaltedIT |
| | phoenix.end2end.index.GlobalMutableNonTxIndexIT |
| 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-937/3/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/937 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux 694f3db55911 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 | 4.x-PHOENIX-5182 / 605656c |
| 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-937/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-937/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-937/3/testReport/ |
| Max. process+thread count | 7048 (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-937/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] swaroopak merged pull request #937: PHOENIX-6198 Add option to IndexTool to specify the source table for scan
Posted by GitBox <gi...@apache.org>.
swaroopak merged pull request #937:
URL: https://github.com/apache/phoenix/pull/937
----------------------------------------------------------------
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] tkhurana commented on a change in pull request #937: PHOENIX-6198 Add option to IndexTool to specify the source table for scan
Posted by GitBox <gi...@apache.org>.
tkhurana commented on a change in pull request #937:
URL: https://github.com/apache/phoenix/pull/937#discussion_r514599687
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/mapreduce/util/PhoenixConfigurationUtil.java
##########
@@ -660,6 +663,19 @@ public static String getIndexToolIndexTableName(Configuration configuration) {
return configuration.get(INDEX_TOOL_INDEX_TABLE_NAME);
}
+ public static void setIndexToolSourceTable(Configuration configuration,
+ IndexScrutinyTool.SourceTable sourceTable) {
+ Preconditions.checkNotNull(configuration);
+ Preconditions.checkNotNull(sourceTable);
+ configuration.set(INDEX_TOOL_SOURCE_TABLE, sourceTable.name());
+ }
+
+ public static IndexScrutinyTool.SourceTable getIndexToolSourceTable(Configuration configuration) {
+ Preconditions.checkNotNull(configuration);
+ return IndexScrutinyTool.SourceTable.valueOf(configuration.get(INDEX_TOOL_SOURCE_TABLE,
+ IndexScrutinyTool.SourceTable.DATA_TABLE_SOURCE.name()));
Review comment:
By default, we are going to use the data table as the source which is what the current behavior is. We have to return a string from which the appropriate enum is constructed. I am not sure what is the question here.
----------------------------------------------------------------
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 #937: PHOENIX-6198 Add option to IndexTool to specify the source table for scan
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #937:
URL: https://github.com/apache/phoenix/pull/937#issuecomment-715565204
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 40s | 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 2 new or modified test files. |
||| _ 4.x Compile Tests _ |
| +1 :green_heart: | mvninstall | 10m 46s | 4.x passed |
| +1 :green_heart: | compile | 0m 54s | 4.x passed |
| +1 :green_heart: | checkstyle | 0m 47s | 4.x passed |
| +1 :green_heart: | javadoc | 0m 44s | 4.x passed |
| +0 :ok: | spotbugs | 3m 16s | phoenix-core in 4.x has 954 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 5m 54s | the patch passed |
| +1 :green_heart: | compile | 0m 58s | the patch passed |
| +1 :green_heart: | javac | 0m 58s | the patch passed |
| -1 :x: | checkstyle | 0m 48s | phoenix-core: The patch generated 30 new + 935 unchanged - 1 fixed = 965 total (was 936) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 0m 44s | the patch passed |
| +1 :green_heart: | spotbugs | 3m 12s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 138m 16s | phoenix-core in the patch failed. |
| +1 :green_heart: | asflicense | 0m 33s | The patch does not generate ASF License warnings. |
| | | 170m 20s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | phoenix.end2end.index.MutableIndexReplicationIT |
| | phoenix.end2end.MaxLookbackIT |
| | phoenix.end2end.StoreNullsIT |
| | phoenix.end2end.OrphanViewToolIT |
| 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-937/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/937 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux 3fb781ef8aab 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 | 4.x / 605656c |
| 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-937/1/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-937/1/artifact/yetus-general-check/output/patch-unit-phoenix-core.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-937/1/testReport/ |
| Max. process+thread count | 7091 (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-937/1/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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] tkhurana commented on pull request #937: PHOENIX-6198 Add option to IndexTool to specify the source table for scan
Posted by GitBox <gi...@apache.org>.
tkhurana commented on pull request #937:
URL: https://github.com/apache/phoenix/pull/937#issuecomment-715471559
@kadirozde @gokceni @priyankporwal @swaroopak
----------------------------------------------------------------
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] tkhurana commented on a change in pull request #937: PHOENIX-6198 Add option to IndexTool to specify the source table for scan
Posted by GitBox <gi...@apache.org>.
tkhurana commented on a change in pull request #937:
URL: https://github.com/apache/phoenix/pull/937#discussion_r511089761
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/mapreduce/index/IndexTool.java
##########
@@ -192,6 +191,14 @@ public static IndexDisableLoggingType fromValue(byte[] value) {
}
}
+ /**
+ * Which table to use as the source table
+ */
+ public enum SourceTable {
Review comment:
It has a BOTH option which is not supported that is why I didn't want to use it.
----------------------------------------------------------------
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] gokceni commented on a change in pull request #937: PHOENIX-6198 Add option to IndexTool to specify the source table for scan
Posted by GitBox <gi...@apache.org>.
gokceni commented on a change in pull request #937:
URL: https://github.com/apache/phoenix/pull/937#discussion_r511031557
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/mapreduce/index/IndexTool.java
##########
@@ -192,6 +191,14 @@ public static IndexDisableLoggingType fromValue(byte[] value) {
}
}
+ /**
+ * Which table to use as the source table
+ */
+ public enum SourceTable {
Review comment:
nit: We have a similar enum in the IndexScrutinyTool. Can we re use it?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/mapreduce/index/IndexTool.java
##########
@@ -285,6 +293,11 @@ public static IndexDisableLoggingType fromValue(byte[] value) {
, "Disable logging of failed verification rows for BEFORE, " +
"AFTER, or BOTH verify jobs");
+ private static final Option USE_INDEX_TABLE_AS_SOURCE_OPTION =
+ new Option("fi", "from-index", false,
+ "To verify every row in the index table has a corresponding row in the data table. " +
+ "Only supported for global indexes. This option is applicable to BEFORE and ONLY type verification jobs");
Review comment:
I think stating this is clearer: This option is applicable only when -v ONLY or -v BEFORE is provided.
----------------------------------------------------------------
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] swaroopak commented on a change in pull request #937: PHOENIX-6198 Add option to IndexTool to specify the source table for scan
Posted by GitBox <gi...@apache.org>.
swaroopak commented on a change in pull request #937:
URL: https://github.com/apache/phoenix/pull/937#discussion_r511094066
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/mapreduce/index/IndexTool.java
##########
@@ -192,6 +191,14 @@ public static IndexDisableLoggingType fromValue(byte[] value) {
}
}
+ /**
+ * Which table to use as the source table
+ */
+ public enum SourceTable {
Review comment:
I agree with Gokcen, does not hurt to use it. In the future, we may want to use both. Would be a good extension.
----------------------------------------------------------------
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