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 2021/05/10 18:23:55 UTC
[GitHub] [phoenix] gokceni opened a new pull request #1228: PHOENIX-6462 Index build that failed should be reporting it into the PHOENIX_INDEX_TOOL_RESULT table
gokceni opened a new pull request #1228:
URL: https://github.com/apache/phoenix/pull/1228
@kadirozde @tkhurana This is for handling the rebuild exception case where we mark the region to be retried
--
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 #1228: PHOENIX-6462 Index build that failed should be reporting it into the PHOENIX_INDEX_TOOL_RESULT table
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #1228:
URL: https://github.com/apache/phoenix/pull/1228#issuecomment-837162579
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 5m 36s | 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 :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. |
||| _ 4.x Compile Tests _ |
| +1 :green_heart: | mvninstall | 14m 15s | 4.x passed |
| +1 :green_heart: | compile | 1m 2s | 4.x passed |
| +1 :green_heart: | checkstyle | 1m 6s | 4.x passed |
| +1 :green_heart: | javadoc | 0m 46s | 4.x passed |
| +0 :ok: | spotbugs | 3m 0s | phoenix-core in 4.x has 945 extant spotbugs warnings. |
| -0 :warning: | patch | 3m 8s | 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 | 5m 44s | the patch passed |
| +1 :green_heart: | compile | 1m 2s | the patch passed |
| -1 :x: | javac | 1m 2s | phoenix-core generated 1 new + 4 unchanged - 0 fixed = 5 total (was 4) |
| -1 :x: | checkstyle | 1m 5s | phoenix-core: The patch generated 35 new + 1539 unchanged - 13 fixed = 1574 total (was 1552) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 0m 45s | the patch passed |
| -1 :x: | spotbugs | 3m 12s | phoenix-core generated 1 new + 945 unchanged - 0 fixed = 946 total (was 945) |
||| _ Other Tests _ |
| -1 :x: | unit | 1m 30s | phoenix-core in the patch failed. |
| +1 :green_heart: | asflicense | 0m 10s | The patch does not generate ASF License warnings. |
| | | 40m 39s | |
| Reason | Tests |
|-------:|:------|
| FindBugs | module:phoenix-core |
| | org.apache.phoenix.mapreduce.index.IndexVerificationResultRepository.SHOULD_RETRY_BYTES is a mutable array At IndexVerificationResultRepository.java: At IndexVerificationResultRepository.java:[line 62] |
| Failed junit tests | phoenix.index.ShouldVerifyTest |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1228/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/1228 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux 6316d20bf9ce 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev/phoenix-personality.sh |
| git revision | 4.x / 6ac64c4 |
| Default Java | Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08 |
| javac | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1228/1/artifact/yetus-general-check/output/diff-compile-javac-phoenix-core.txt |
| checkstyle | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1228/1/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt |
| spotbugs | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1228/1/artifact/yetus-general-check/output/new-spotbugs-phoenix-core.html |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1228/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-1228/1/testReport/ |
| Max. process+thread count | 463 (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-1228/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] gokceni commented on a change in pull request #1228: PHOENIX-6462 Index build that failed should be reporting it into the PHOENIX_INDEX_TOOL_RESULT table
Posted by GitBox <gi...@apache.org>.
gokceni commented on a change in pull request #1228:
URL: https://github.com/apache/phoenix/pull/1228#discussion_r630392682
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -357,6 +361,7 @@ public boolean next(List<Cell> results) throws IOException {
} catch (Throwable e) {
LOGGER.error("Exception in IndexRebuildRegionScanner for region "
+ region.getRegionInfo().getRegionNameAsString(), e);
+ this.shouldRetry = true;
Review comment:
This is a good idea! @kadirozde what do you think? We can just say, ok we know that we are successfully finished for this region and shouldRetry=false in that end of try, and all other cases there must be either region split/move or exception and retry.
--
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 #1228: PHOENIX-6462 Index build that failed should be reporting it into the PHOENIX_INDEX_TOOL_RESULT table
Posted by GitBox <gi...@apache.org>.
gokceni commented on a change in pull request #1228:
URL: https://github.com/apache/phoenix/pull/1228#discussion_r631299501
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/GlobalIndexRegionScanner.java
##########
@@ -408,7 +413,7 @@ public void close() throws IOException {
try {
if (verificationResultRepository != null) {
verificationResultRepository.logToIndexToolResultTable(verificationResult,
- verifyType, region.getRegionInfo().getRegionName(), skipped);
+ verifyType, region.getRegionInfo().getRegionName(), skipped, shouldRetry);
Review comment:
Wouln't it be too chatty? I added logging for the true case already. I think it should be enough
--
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 #1228: PHOENIX-6462 Index build that failed should be reporting it into the PHOENIX_INDEX_TOOL_RESULT table
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #1228:
URL: https://github.com/apache/phoenix/pull/1228#issuecomment-840103664
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 28s | 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 :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. |
||| _ 4.x Compile Tests _ |
| +1 :green_heart: | mvninstall | 13m 59s | 4.x passed |
| +1 :green_heart: | compile | 0m 59s | 4.x passed |
| +1 :green_heart: | checkstyle | 1m 5s | 4.x passed |
| +1 :green_heart: | javadoc | 0m 46s | 4.x passed |
| +0 :ok: | spotbugs | 3m 4s | phoenix-core in 4.x has 945 extant spotbugs warnings. |
| -0 :warning: | patch | 3m 11s | 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 | 5m 43s | the patch passed |
| +1 :green_heart: | compile | 1m 0s | the patch passed |
| -1 :x: | javac | 1m 0s | phoenix-core generated 1 new + 4 unchanged - 0 fixed = 5 total (was 4) |
| -1 :x: | checkstyle | 1m 6s | phoenix-core: The patch generated 29 new + 1546 unchanged - 6 fixed = 1575 total (was 1552) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 0m 44s | the patch passed |
| -1 :x: | spotbugs | 3m 17s | phoenix-core generated 1 new + 945 unchanged - 0 fixed = 946 total (was 945) |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 137m 35s | phoenix-core in the patch passed. |
| +1 :green_heart: | asflicense | 0m 10s | The patch does not generate ASF License warnings. |
| | | 170m 58s | |
| Reason | Tests |
|-------:|:------|
| FindBugs | module:phoenix-core |
| | org.apache.phoenix.mapreduce.index.IndexVerificationResultRepository.SHOULD_RETRY_BYTES is a mutable array At IndexVerificationResultRepository.java: At IndexVerificationResultRepository.java:[line 62] |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1228/2/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/1228 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux 95bded663c9f 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev/phoenix-personality.sh |
| git revision | 4.x / 6ac64c4 |
| Default Java | Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08 |
| javac | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1228/2/artifact/yetus-general-check/output/diff-compile-javac-phoenix-core.txt |
| checkstyle | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1228/2/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt |
| spotbugs | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1228/2/artifact/yetus-general-check/output/new-spotbugs-phoenix-core.html |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1228/2/testReport/ |
| Max. process+thread count | 5707 (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-1228/2/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
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] kadirozde commented on a change in pull request #1228: PHOENIX-6462 Index build that failed should be reporting it into the PHOENIX_INDEX_TOOL_RESULT table
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #1228:
URL: https://github.com/apache/phoenix/pull/1228#discussion_r630629270
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -357,6 +361,7 @@ public boolean next(List<Cell> results) throws IOException {
} catch (Throwable e) {
LOGGER.error("Exception in IndexRebuildRegionScanner for region "
+ region.getRegionInfo().getRegionNameAsString(), e);
+ this.shouldRetry = true;
Review comment:
I think instead of the exception approach and defining a new field shouldRetry, we can use the existing hasMore field and pass hasMore instead shouldRetry to logToIndexToolResultTable().
--
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 #1228: PHOENIX-6462 Index build that failed should be reporting it into the PHOENIX_INDEX_TOOL_RESULT table
Posted by GitBox <gi...@apache.org>.
gokceni commented on a change in pull request #1228:
URL: https://github.com/apache/phoenix/pull/1228#discussion_r630606919
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -357,6 +361,7 @@ public boolean next(List<Cell> results) throws IOException {
} catch (Throwable e) {
LOGGER.error("Exception in IndexRebuildRegionScanner for region "
+ region.getRegionInfo().getRegionNameAsString(), e);
+ this.shouldRetry = true;
Review comment:
Actually this is the only path where we set this variable. Any exception in verify or rebuild or region split or move in this loop will set it. I am not sure how we can detect just the successful case since we don't know the success case as you said. What do we consider as success case? So I think leaving it like this will cover most of the exception cases.
--
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 #1228: PHOENIX-6462 Index build that failed should be reporting it into the PHOENIX_INDEX_TOOL_RESULT table
Posted by GitBox <gi...@apache.org>.
tkhurana commented on a change in pull request #1228:
URL: https://github.com/apache/phoenix/pull/1228#discussion_r630731025
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/GlobalIndexRegionScanner.java
##########
@@ -408,7 +413,7 @@ public void close() throws IOException {
try {
if (verificationResultRepository != null) {
verificationResultRepository.logToIndexToolResultTable(verificationResult,
- verifyType, region.getRegionInfo().getRegionName(), skipped);
+ verifyType, region.getRegionInfo().getRegionName(), skipped, shouldRetry);
Review comment:
Can we add a log here. It will be helpful to know the value of shouldRetry.
--
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 #1228: PHOENIX-6462 Index build that failed should be reporting it into the PHOENIX_INDEX_TOOL_RESULT table
Posted by GitBox <gi...@apache.org>.
tkhurana commented on a change in pull request #1228:
URL: https://github.com/apache/phoenix/pull/1228#discussion_r630372873
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -357,6 +361,7 @@ public boolean next(List<Cell> results) throws IOException {
} catch (Throwable e) {
LOGGER.error("Exception in IndexRebuildRegionScanner for region "
+ region.getRegionInfo().getRegionNameAsString(), e);
+ this.shouldRetry = true;
Review comment:
Are we sure this is the only path where we have to set this variable ? Are there no other exception paths ? In general it is difficult to catch all exception paths. An alternative could be to just handle the successful case and retry all other times.
--
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 #1228: PHOENIX-6462 Index build that failed should be reporting it into the PHOENIX_INDEX_TOOL_RESULT table
Posted by GitBox <gi...@apache.org>.
tkhurana commented on a change in pull request #1228:
URL: https://github.com/apache/phoenix/pull/1228#discussion_r630731549
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/GlobalIndexRegionScanner.java
##########
@@ -373,7 +374,11 @@ public boolean shouldVerify() throws IOException {
if(verificationResultTemp != null) {
verificationResult = verificationResultTemp;
}
+
Review comment:
There should also be some logging when we are skipping rebuilding the region
--
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