Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

PHOENIX-6055: Not matching index mutation error needs to report more … #891

Closed
wants to merge 1 commit into from

Conversation

gokceni
Copy link
Contributor

@gokceni gokceni commented Sep 22, 2020

…information rather than first expected mutation

@gokceni
Copy link
Contributor Author

gokceni commented Sep 22, 2020

@kadirozde

Copy link
Contributor

@gjacoby126 gjacoby126 left a comment

Choose a reason for hiding this comment

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

+1

@@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

@kadirozde kadirozde Sep 23, 2020

Choose a reason for hiding this comment

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

@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.

…information rather than first expected mutation
Copy link
Contributor

@kadirozde kadirozde left a comment

Choose a reason for hiding this comment

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

+1

@stoty
Copy link
Contributor

stoty commented Sep 28, 2020

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 0s Docker mode activated.
-1 ❌ patch 0m 4s #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 #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.

@gjacoby126
Copy link
Contributor

JIRA is resolved, closing PR

@gjacoby126 gjacoby126 closed this Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants