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/09/22 23:04:31 UTC
[GitHub] [phoenix] gokceni opened a new pull request #891: PHOENIX-6055: Not matching index mutation error needs to report more …
gokceni opened a new pull request #891:
URL: https://github.com/apache/phoenix/pull/891
…information rather than first expected mutation
----------------------------------------------------------------
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 pull request #891: PHOENIX-6055: Not matching index mutation error needs to report more …
Posted by GitBox <gi...@apache.org>.
gokceni commented on pull request #891:
URL: https://github.com/apache/phoenix/pull/891#issuecomment-697027653
@kadirozde
----------------------------------------------------------------
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 #891: PHOENIX-6055: Not matching index mutation error needs to report more …
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #891:
URL: https://github.com/apache/phoenix/pull/891#discussion_r493785508
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -768,7 +768,8 @@ public boolean verifySingleIndexRow(Result indexRow, IndexToolVerificationResult
}
} else {
byte[] dataKey = indexMaintainer.buildDataRowKey(new ImmutableBytesWritable(indexRow.getRow()), viewConstants);
- String errorMsg = "Not matching index row";
+ String errorMsg = String.format("Not matching index row. matchingCount=0. expectedIndex=%d. expectedMutationSize=%d. actualIndex=%d. actualMutationSize=%d. expectedTs=%d. actualTs=%d",
Review comment:
@gokceni, this fix is not correct and will not be very useful for troubleshooting. First, the check actual == null is not meaningful here. You need to check if actualIndex < actualSIze then actual is useful and the we should report the type of actual (put or delete) and the timestamp of the actual. I still think we absolutely should not include the timestamps in the error message as we have separate columns for them. In the following line expectedMutationList.get(0)) should be replaced by getTimestamp(expected)). expected cannot be null here. If actualIndex < actualSIze then getTimestamp(actual)) should be reported instead of 0L.
----------------------------------------------------------------
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 pull request #891: PHOENIX-6055: Not matching index mutation error needs to report more …
Posted by GitBox <gi...@apache.org>.
gokceni commented on pull request #891:
URL: https://github.com/apache/phoenix/pull/891#issuecomment-697027653
@kadirozde
----------------------------------------------------------------
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 #891: PHOENIX-6055: Not matching index mutation error needs to report more …
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #891:
URL: https://github.com/apache/phoenix/pull/891#discussion_r493216064
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -768,7 +768,8 @@ public boolean verifySingleIndexRow(Result indexRow, IndexToolVerificationResult
}
} else {
byte[] dataKey = indexMaintainer.buildDataRowKey(new ImmutableBytesWritable(indexRow.getRow()), viewConstants);
- String errorMsg = "Not matching index row";
+ String errorMsg = String.format("Not matching index row. matchingCount=0. expectedIndex=%d. expectedMutationSize=%d. actualIndex=%d. actualMutationSize=%d. expectedTs=%d. actualTs=%d",
Review comment:
The version of this file is not the most recent as I noticed that it does not have the MaxLookBack changes. The patch with the latest version should be different, I think. Also, there is no need to include expected and actual timestamps in the error message since they are reported as separate columns in the output table. We should fix those reported values instead. Please use the most recent version of IndexRebuildRegionScanner.java and resubmit your patch.
----------------------------------------------------------------
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 closed pull request #891: PHOENIX-6055: Not matching index mutation error needs to report more …
Posted by GitBox <gi...@apache.org>.
gjacoby126 closed pull request #891:
URL: https://github.com/apache/phoenix/pull/891
----------------------------------------------------------------
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 pull request #891: PHOENIX-6055: Not matching index mutation error needs to report more …
Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on pull request #891:
URL: https://github.com/apache/phoenix/pull/891#issuecomment-740837357
JIRA is resolved, closing PR
----------------------------------------------------------------
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 #891: PHOENIX-6055: Not matching index mutation error needs to report more …
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #891:
URL: https://github.com/apache/phoenix/pull/891#issuecomment-700175676
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 0s | Docker mode activated. |
| -1 :x: | patch | 0m 4s | https://github.com/apache/phoenix/pull/891 does not apply to master. Rebase required? Wrong Branch? See https://yetus.apache.org/documentation/in-progress/precommit-patchnames for help. |
| Subsystem | Report/Notes |
|----------:|:-------------|
| GITHUB PR | https://github.com/apache/phoenix/pull/891 |
| JIRA Issue | PHOENIX-6055 |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-891/1/console |
| versions | git=2.17.1 |
| 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 #891: PHOENIX-6055: Not matching index mutation error needs to report more …
Posted by GitBox <gi...@apache.org>.
gokceni commented on a change in pull request #891:
URL: https://github.com/apache/phoenix/pull/891#discussion_r493745610
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -768,7 +768,8 @@ public boolean verifySingleIndexRow(Result indexRow, IndexToolVerificationResult
}
} else {
byte[] dataKey = indexMaintainer.buildDataRowKey(new ImmutableBytesWritable(indexRow.getRow()), viewConstants);
- String errorMsg = "Not matching index row";
+ String errorMsg = String.format("Not matching index row. matchingCount=0. expectedIndex=%d. expectedMutationSize=%d. actualIndex=%d. actualMutationSize=%d. expectedTs=%d. actualTs=%d",
Review comment:
@kadirozde Rebased. I don't think we put the actualTs since in the next line we always put 0L. For the expected, since we put expectedMutations.get(0) I don't think it hurts to put what it is here in case we change the above loop and since we put the actualTs. what do you think?
----------------------------------------------------------------
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 #891: PHOENIX-6055: Not matching index mutation error needs to report more …
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #891:
URL: https://github.com/apache/phoenix/pull/891#discussion_r493785508
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -768,7 +768,8 @@ public boolean verifySingleIndexRow(Result indexRow, IndexToolVerificationResult
}
} else {
byte[] dataKey = indexMaintainer.buildDataRowKey(new ImmutableBytesWritable(indexRow.getRow()), viewConstants);
- String errorMsg = "Not matching index row";
+ String errorMsg = String.format("Not matching index row. matchingCount=0. expectedIndex=%d. expectedMutationSize=%d. actualIndex=%d. actualMutationSize=%d. expectedTs=%d. actualTs=%d",
Review comment:
@gokceni, this fix is not correct and will not be very useful for troubleshooting. First, the check actual == null is not meaningful here. You need to check if actualIndex < actualSIze then actual is useful and we should report the type of actual (put or delete) and the timestamp of the actual. I still think we absolutely should not include the timestamps in the error message as we have separate columns for them. In the following line expectedMutationList.get(0)) should be replaced by getTimestamp(expected)). expected cannot be null here. If actualIndex < actualSIze then getTimestamp(actual)) should be reported instead of 0L.
----------------------------------------------------------------
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