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/03/24 12:00:13 UTC
[GitHub] [phoenix] kadirozde opened a new pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
kadirozde opened a new pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741
…updates
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r397521193
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // find the first (latest) actual unverified put mutation
+ Mutation actual = null;
+ for (Mutation mutation : actualMutationList) {
+ if (mutation instanceof Put && !isVerified((Put) mutation)) {
+ actual = mutation;
+ break;
+ }
+ }
+ if (actual == null) {
+ return;
+ }
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ return;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
+ }
+ Collections.sort(actualMutationList, MUTATION_TS_DESC_COMPARATOR);
+ }
+
+ private void cleanUpActualMutationList(List<Mutation> actualMutationList)
+ throws IOException {
+ Iterator<Mutation> iterator = actualMutationList.iterator();
+ Mutation previous = null;
+ while (iterator.hasNext()) {
+ Mutation mutation = iterator.next();
+ if ((mutation instanceof Put && !isVerified((Put) mutation)) ||
+ (mutation instanceof Delete && isDeleteFamilyVersion(mutation))) {
+ iterator.remove();
+ } else {
+ if (previous != null && getTimestamp(previous) == getTimestamp(mutation) &&
+ ((previous instanceof Put && mutation instanceof Put) ||
+ previous instanceof Delete && mutation instanceof Delete)) {
+ iterator.remove();
+ } else {
+ previous = mutation;
+ }
+ }
+ }
+ }
+
/**
- * indexRow is the set of all cells of all the row version of an index row from the index table. These are actual
- * cells. We group these cells based on timestamp and type (put vs delete), and form the actual set of
- * index mutations. indexKeyToMutationMap is a map from an index row key to a set of mutations that are generated
- * using the rebuild process (i.e., by replaying raw data table mutations). These sets are sets of expected
- * index mutations, one set for each index row key. Since not all mutations in the index table have both phase
- * (i.e., pre and post data phase) mutations, we cannot compare actual index mutations with expected one by one
- * and expect to find them identical. We need to consider concurrent data mutation effects, data table row write
- * failures, post index write failures. Thus, we need to allow some expected and actual mutations to be skipped
- * during comparing actual mutations to index mutations.
+ * There are two types of verification: without repair and with repair. Without-repair verification is done before
+ * or after index rebuild. It is done before index rebuild to identify the rows to be rebuilt. It is done after
+ * index rebuild to verify the rows that have been rebuilt. With-repair verification can be done anytime using
+ * the “-v ONLY” option to check the consistency of the index table.
+ *
+ * Unverified Rows
+ *
+ * For each mutable data table mutation during regular data table updates, two operations are done on the data table.
+ * One is to read the existing row state, and the second is to update the data table for this row. The processing of
+ * concurrent data mutations are serialized once for reading the existing row states, and then serialized again
+ * for updating the data table. In other words, they go through locking twice, i.e., [lock, read, unlock] and
+ * [lock, write, unlock]. Because of this two phase locking, for a pair of concurrent mutations (for the same row),
+ * the same row state can be read from the data table. This means the same existing index row can be made unverified
+ * twice with different timestamps, one for each concurrent mutation. These unverified mutations can be repaired
+ * from the data table later during HBase scans using the index read repair process. This is one of the reasons
+ * for having extra unverified rows in the index table. The other reason is the data table write failures.
+ * When a data table write fails, it leaves an unverified index row behind. These rows are never returned to clients,
+ * instead they are repaired, which means either they are rebuilt from their data table rows or they are deleted if
+ * their data table rows do not exist.
+ *
+ * Delete Family Version Markers
+ *
+ * The family version delete markers are generated by the read repair to remove extra unverified rows. They only
Review comment:
delete marks is not only generated by the read repair. If there is a column delete on the indexed column in data table, there will be family version delete marker as well. Will it be a problem?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] kadirozde commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r398907520
##########
File path: phoenix-core/src/test/java/org/apache/phoenix/index/VerifySingleIndexRowTest.java
##########
@@ -350,6 +351,7 @@ public void testVerifySingleIndexRow_allUnverified() throws IOException {
}
}
+ @Ignore
Review comment:
They will be fixed later. I will either fix it in this PR or let @swaroopak fix them in a separate jira. I needed to disable it to submit the patch for pre-commit testing
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r398344146
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -613,39 +605,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
}
return getMutationsWithSameTS(put, del);
}
+ /**
+ * In this method, the actual list is repaired in memory using the expected list which is actually the output of
+ * rebuilding the index table row. The result of this repair is used only for verification.
+ */
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // Find the first (latest) actual unverified put mutation
+ for (Mutation actual : actualMutationList) {
+ if (actual instanceof Put && !isVerified((Put) actual)) {
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ continue;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
Review comment:
will it be a problem to modify the list when iterating it? I am not an expert on Java, just a guess here.
https://stackoverflow.com/questions/6866238/concurrent-modification-exception-adding-to-an-arraylist
https://www.baeldung.com/java-concurrentmodificationexception
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r398944198
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // find the first (latest) actual unverified put mutation
+ Mutation actual = null;
+ for (Mutation mutation : actualMutationList) {
+ if (mutation instanceof Put && !isVerified((Put) mutation)) {
+ actual = mutation;
+ break;
+ }
+ }
+ if (actual == null) {
+ return;
+ }
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ return;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
+ }
+ Collections.sort(actualMutationList, MUTATION_TS_DESC_COMPARATOR);
+ }
+
+ private void cleanUpActualMutationList(List<Mutation> actualMutationList)
+ throws IOException {
+ Iterator<Mutation> iterator = actualMutationList.iterator();
+ Mutation previous = null;
+ while (iterator.hasNext()) {
+ Mutation mutation = iterator.next();
+ if ((mutation instanceof Put && !isVerified((Put) mutation)) ||
+ (mutation instanceof Delete && isDeleteFamilyVersion(mutation))) {
+ iterator.remove();
Review comment:
Yes, this corresponding index mutation will be in the expected mutation list, but it will not find the matching one in the actual list, as the unverified mutation will be removed 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
With regards,
Apache Git Services
[GitHub] [phoenix] kadirozde commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r398349866
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -613,39 +605,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
}
return getMutationsWithSameTS(put, del);
}
+ /**
+ * In this method, the actual list is repaired in memory using the expected list which is actually the output of
+ * rebuilding the index table row. The result of this repair is used only for verification.
+ */
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // Find the first (latest) actual unverified put mutation
+ for (Mutation actual : actualMutationList) {
+ if (actual instanceof Put && !isVerified((Put) actual)) {
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ continue;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
Review comment:
Yes, this should be a problem. Good catch! I will fix 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
With regards,
Apache Git Services
[GitHub] [phoenix] wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r397523167
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // find the first (latest) actual unverified put mutation
+ Mutation actual = null;
+ for (Mutation mutation : actualMutationList) {
+ if (mutation instanceof Put && !isVerified((Put) mutation)) {
+ actual = mutation;
+ break;
+ }
+ }
+ if (actual == null) {
+ return;
+ }
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ return;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
+ }
+ Collections.sort(actualMutationList, MUTATION_TS_DESC_COMPARATOR);
+ }
+
+ private void cleanUpActualMutationList(List<Mutation> actualMutationList)
+ throws IOException {
+ Iterator<Mutation> iterator = actualMutationList.iterator();
+ Mutation previous = null;
+ while (iterator.hasNext()) {
+ Mutation mutation = iterator.next();
+ if ((mutation instanceof Put && !isVerified((Put) mutation)) ||
+ (mutation instanceof Delete && isDeleteFamilyVersion(mutation))) {
+ iterator.remove();
+ } else {
+ if (previous != null && getTimestamp(previous) == getTimestamp(mutation) &&
+ ((previous instanceof Put && mutation instanceof Put) ||
+ previous instanceof Delete && mutation instanceof Delete)) {
+ iterator.remove();
+ } else {
+ previous = mutation;
+ }
+ }
+ }
+ }
+
/**
- * indexRow is the set of all cells of all the row version of an index row from the index table. These are actual
- * cells. We group these cells based on timestamp and type (put vs delete), and form the actual set of
- * index mutations. indexKeyToMutationMap is a map from an index row key to a set of mutations that are generated
- * using the rebuild process (i.e., by replaying raw data table mutations). These sets are sets of expected
- * index mutations, one set for each index row key. Since not all mutations in the index table have both phase
- * (i.e., pre and post data phase) mutations, we cannot compare actual index mutations with expected one by one
- * and expect to find them identical. We need to consider concurrent data mutation effects, data table row write
- * failures, post index write failures. Thus, we need to allow some expected and actual mutations to be skipped
- * during comparing actual mutations to index mutations.
+ * There are two types of verification: without repair and with repair. Without-repair verification is done before
+ * or after index rebuild. It is done before index rebuild to identify the rows to be rebuilt. It is done after
+ * index rebuild to verify the rows that have been rebuilt. With-repair verification can be done anytime using
+ * the “-v ONLY” option to check the consistency of the index table.
+ *
+ * Unverified Rows
+ *
+ * For each mutable data table mutation during regular data table updates, two operations are done on the data table.
+ * One is to read the existing row state, and the second is to update the data table for this row. The processing of
+ * concurrent data mutations are serialized once for reading the existing row states, and then serialized again
+ * for updating the data table. In other words, they go through locking twice, i.e., [lock, read, unlock] and
+ * [lock, write, unlock]. Because of this two phase locking, for a pair of concurrent mutations (for the same row),
+ * the same row state can be read from the data table. This means the same existing index row can be made unverified
+ * twice with different timestamps, one for each concurrent mutation. These unverified mutations can be repaired
+ * from the data table later during HBase scans using the index read repair process. This is one of the reasons
+ * for having extra unverified rows in the index table. The other reason is the data table write failures.
+ * When a data table write fails, it leaves an unverified index row behind. These rows are never returned to clients,
+ * instead they are repaired, which means either they are rebuilt from their data table rows or they are deleted if
+ * their data table rows do not exist.
+ *
+ * Delete Family Version Markers
+ *
+ * The family version delete markers are generated by the read repair to remove extra unverified rows. They only
+ * show up in the actual mutation list since they are not generated for regular table updates or index rebuilds.
+ * For the verification purpose, these delete markers can be treated as extra unverified rows and can be safely
+ * skipped.
+ *
+ * Delete Family Markers
+ * Delete family markers are generated during read repair, regular table updates and index rebuilds to delete index
+ * table rows. The read repair generates them to delete extra unverified rows. During regular table updates or
+ * index rebuilds, the delete family markers are used to delete index rows due to data table row deletes or
+ * data table row overwrites.
+ *
+ * Verification Algorithm
+ *
+ * IndexTool verification generates an expected list of index mutations from the data table rows and uses this list
+ * to check if index table rows are consistent with the data table.
*
- * The main idea for the verification algorithm used here is to match every expected verified put with an actual
- * put such that these two mutations are the same except that actual mutation can be unverified.
+ * The expect list is generated using the index rebuild algorithm. This mean for a given row, the list can include
+ * a number of put and delete mutations such that the followings hold:
*
- * Some background on why we can skip some of the actual unverified puts and delete markers due to concurrent data
- * table updates is as follows:
+ * Every mutation will include a set of cells with the same timestamp
+ * Every mutation has a different timestamp
+ * A delete mutation will include only delete family cells and it is for deleting the entire row and its versions
+ * Every put mutation is verified
*
- * For each data table mutation, two operations are done on the data table. One is to read the existing row state,
- * and the second is to write to the data table. The processing of concurrent data mutations are serialized once
- * for reading the existing row states, and then serialized again for updating data table. In other words,
- * they go through locking twice, i.e., [lock, read, unlock] and [lock, write, unlock]. Because of this two phase
- * locking, for a pair of concurrent mutations (for the same row), the same row state can be read from the data
- * table. This means the same existing index row can be made unverified twice with different timestamps, one for
- * each concurrent mutation. These unverified mutation are then repaired from the data table. Since expected
- * mutations are used for rebuild (which is also used by the read repair), skipping these unverified put mutations
- * that are not matching with expected mutation are safe as they will go through the same process during
- * read repair and will be skipped and eventually cleaned up by the read repair. We can skip the delete markers
- * safely too as they are placed to clean up these unverified mutations. When the data table rows are rebuilt,
- * the rebuild process generates the delete family markers. The timestamp of delete markers are the timestamp of
- * the data table mutation for which the delete marker is added. Thus, the timestamp of these delete markers will be
- * higher than the timestamp of index row to be deleted.
+ * For both verification types, after the expected list of index mutations is constructed for a given data table,
+ * another list called the actual list of index mutations is constructed by reading the index table row using HBase
+ * raw scan and all versions of the cells of the row are retrieved.
+ *
+ * As in the construction for the expected list, the cells are grouped into a put and a delete set. The put and
+ * delete sets for a given row are further grouped based on their timestamps into put and delete mutations such that
+ * all the cells in a mutation have the timestamps. The put and delete mutations are then sorted within a single
+ * list. Mutations in this list are sorted in ascending order of their timestamp. This list is the actual list.
+ *
+ * For the without-repair verification, unverified mutations and family version delete markers are removed from
Review comment:
will it remove the innocent delete markers as my previous comment?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r397523167
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // find the first (latest) actual unverified put mutation
+ Mutation actual = null;
+ for (Mutation mutation : actualMutationList) {
+ if (mutation instanceof Put && !isVerified((Put) mutation)) {
+ actual = mutation;
+ break;
+ }
+ }
+ if (actual == null) {
+ return;
+ }
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ return;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
+ }
+ Collections.sort(actualMutationList, MUTATION_TS_DESC_COMPARATOR);
+ }
+
+ private void cleanUpActualMutationList(List<Mutation> actualMutationList)
+ throws IOException {
+ Iterator<Mutation> iterator = actualMutationList.iterator();
+ Mutation previous = null;
+ while (iterator.hasNext()) {
+ Mutation mutation = iterator.next();
+ if ((mutation instanceof Put && !isVerified((Put) mutation)) ||
+ (mutation instanceof Delete && isDeleteFamilyVersion(mutation))) {
+ iterator.remove();
+ } else {
+ if (previous != null && getTimestamp(previous) == getTimestamp(mutation) &&
+ ((previous instanceof Put && mutation instanceof Put) ||
+ previous instanceof Delete && mutation instanceof Delete)) {
+ iterator.remove();
+ } else {
+ previous = mutation;
+ }
+ }
+ }
+ }
+
/**
- * indexRow is the set of all cells of all the row version of an index row from the index table. These are actual
- * cells. We group these cells based on timestamp and type (put vs delete), and form the actual set of
- * index mutations. indexKeyToMutationMap is a map from an index row key to a set of mutations that are generated
- * using the rebuild process (i.e., by replaying raw data table mutations). These sets are sets of expected
- * index mutations, one set for each index row key. Since not all mutations in the index table have both phase
- * (i.e., pre and post data phase) mutations, we cannot compare actual index mutations with expected one by one
- * and expect to find them identical. We need to consider concurrent data mutation effects, data table row write
- * failures, post index write failures. Thus, we need to allow some expected and actual mutations to be skipped
- * during comparing actual mutations to index mutations.
+ * There are two types of verification: without repair and with repair. Without-repair verification is done before
+ * or after index rebuild. It is done before index rebuild to identify the rows to be rebuilt. It is done after
+ * index rebuild to verify the rows that have been rebuilt. With-repair verification can be done anytime using
+ * the “-v ONLY” option to check the consistency of the index table.
+ *
+ * Unverified Rows
+ *
+ * For each mutable data table mutation during regular data table updates, two operations are done on the data table.
+ * One is to read the existing row state, and the second is to update the data table for this row. The processing of
+ * concurrent data mutations are serialized once for reading the existing row states, and then serialized again
+ * for updating the data table. In other words, they go through locking twice, i.e., [lock, read, unlock] and
+ * [lock, write, unlock]. Because of this two phase locking, for a pair of concurrent mutations (for the same row),
+ * the same row state can be read from the data table. This means the same existing index row can be made unverified
+ * twice with different timestamps, one for each concurrent mutation. These unverified mutations can be repaired
+ * from the data table later during HBase scans using the index read repair process. This is one of the reasons
+ * for having extra unverified rows in the index table. The other reason is the data table write failures.
+ * When a data table write fails, it leaves an unverified index row behind. These rows are never returned to clients,
+ * instead they are repaired, which means either they are rebuilt from their data table rows or they are deleted if
+ * their data table rows do not exist.
+ *
+ * Delete Family Version Markers
+ *
+ * The family version delete markers are generated by the read repair to remove extra unverified rows. They only
+ * show up in the actual mutation list since they are not generated for regular table updates or index rebuilds.
+ * For the verification purpose, these delete markers can be treated as extra unverified rows and can be safely
+ * skipped.
+ *
+ * Delete Family Markers
+ * Delete family markers are generated during read repair, regular table updates and index rebuilds to delete index
+ * table rows. The read repair generates them to delete extra unverified rows. During regular table updates or
+ * index rebuilds, the delete family markers are used to delete index rows due to data table row deletes or
+ * data table row overwrites.
+ *
+ * Verification Algorithm
+ *
+ * IndexTool verification generates an expected list of index mutations from the data table rows and uses this list
+ * to check if index table rows are consistent with the data table.
*
- * The main idea for the verification algorithm used here is to match every expected verified put with an actual
- * put such that these two mutations are the same except that actual mutation can be unverified.
+ * The expect list is generated using the index rebuild algorithm. This mean for a given row, the list can include
+ * a number of put and delete mutations such that the followings hold:
*
- * Some background on why we can skip some of the actual unverified puts and delete markers due to concurrent data
- * table updates is as follows:
+ * Every mutation will include a set of cells with the same timestamp
+ * Every mutation has a different timestamp
+ * A delete mutation will include only delete family cells and it is for deleting the entire row and its versions
+ * Every put mutation is verified
*
- * For each data table mutation, two operations are done on the data table. One is to read the existing row state,
- * and the second is to write to the data table. The processing of concurrent data mutations are serialized once
- * for reading the existing row states, and then serialized again for updating data table. In other words,
- * they go through locking twice, i.e., [lock, read, unlock] and [lock, write, unlock]. Because of this two phase
- * locking, for a pair of concurrent mutations (for the same row), the same row state can be read from the data
- * table. This means the same existing index row can be made unverified twice with different timestamps, one for
- * each concurrent mutation. These unverified mutation are then repaired from the data table. Since expected
- * mutations are used for rebuild (which is also used by the read repair), skipping these unverified put mutations
- * that are not matching with expected mutation are safe as they will go through the same process during
- * read repair and will be skipped and eventually cleaned up by the read repair. We can skip the delete markers
- * safely too as they are placed to clean up these unverified mutations. When the data table rows are rebuilt,
- * the rebuild process generates the delete family markers. The timestamp of delete markers are the timestamp of
- * the data table mutation for which the delete marker is added. Thus, the timestamp of these delete markers will be
- * higher than the timestamp of index row to be deleted.
+ * For both verification types, after the expected list of index mutations is constructed for a given data table,
+ * another list called the actual list of index mutations is constructed by reading the index table row using HBase
+ * raw scan and all versions of the cells of the row are retrieved.
+ *
+ * As in the construction for the expected list, the cells are grouped into a put and a delete set. The put and
+ * delete sets for a given row are further grouped based on their timestamps into put and delete mutations such that
+ * all the cells in a mutation have the timestamps. The put and delete mutations are then sorted within a single
+ * list. Mutations in this list are sorted in ascending order of their timestamp. This list is the actual list.
+ *
+ * For the without-repair verification, unverified mutations and family version delete markers are removed from
Review comment:
will it remove the innocent delete markers as my previous comment?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r398344146
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -613,39 +605,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
}
return getMutationsWithSameTS(put, del);
}
+ /**
+ * In this method, the actual list is repaired in memory using the expected list which is actually the output of
+ * rebuilding the index table row. The result of this repair is used only for verification.
+ */
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // Find the first (latest) actual unverified put mutation
+ for (Mutation actual : actualMutationList) {
+ if (actual instanceof Put && !isVerified((Put) actual)) {
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ continue;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
Review comment:
@kadirozde, will it be a problem to modify the list when iterating it? I am not an expert on Java, just a guess here.
https://stackoverflow.com/questions/6866238/concurrent-modification-exception-adding-to-an-arraylist
https://www.baeldung.com/java-concurrentmodificationexception
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] kadirozde commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r398353963
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -613,39 +605,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
}
return getMutationsWithSameTS(put, del);
}
+ /**
+ * In this method, the actual list is repaired in memory using the expected list which is actually the output of
+ * rebuilding the index table row. The result of this repair is used only for verification.
+ */
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // Find the first (latest) actual unverified put mutation
+ for (Mutation actual : actualMutationList) {
+ if (actual instanceof Put && !isVerified((Put) actual)) {
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ continue;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
Review comment:
This is in general a problem. I was thinking why it did not cause an issue in my tests. The reason is that we are not actually modifying the list here while iterating over it. We modify the list and then terminate the loop. Therefore, it is not a problem. Regardless, I will change the code to eliminate the confusion.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] kadirozde commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r398935028
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // find the first (latest) actual unverified put mutation
+ Mutation actual = null;
+ for (Mutation mutation : actualMutationList) {
+ if (mutation instanceof Put && !isVerified((Put) mutation)) {
+ actual = mutation;
+ break;
+ }
+ }
+ if (actual == null) {
+ return;
+ }
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ return;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
+ }
+ Collections.sort(actualMutationList, MUTATION_TS_DESC_COMPARATOR);
+ }
+
+ private void cleanUpActualMutationList(List<Mutation> actualMutationList)
+ throws IOException {
+ Iterator<Mutation> iterator = actualMutationList.iterator();
+ Mutation previous = null;
+ while (iterator.hasNext()) {
+ Mutation mutation = iterator.next();
+ if ((mutation instanceof Put && !isVerified((Put) mutation)) ||
+ (mutation instanceof Delete && isDeleteFamilyVersion(mutation))) {
+ iterator.remove();
Review comment:
Since the update is made on the data table, the expected list will include the verified index row for this unverified row. So, it will not be missed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r398344146
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -613,39 +605,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
}
return getMutationsWithSameTS(put, del);
}
+ /**
+ * In this method, the actual list is repaired in memory using the expected list which is actually the output of
+ * rebuilding the index table row. The result of this repair is used only for verification.
+ */
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // Find the first (latest) actual unverified put mutation
+ for (Mutation actual : actualMutationList) {
+ if (actual instanceof Put && !isVerified((Put) actual)) {
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ continue;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
Review comment:
will it be a problem to modify the list when iterating it?
https://stackoverflow.com/questions/6866238/concurrent-modification-exception-adding-to-an-arraylist
https://www.baeldung.com/java-concurrentmodificationexception
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r397521193
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // find the first (latest) actual unverified put mutation
+ Mutation actual = null;
+ for (Mutation mutation : actualMutationList) {
+ if (mutation instanceof Put && !isVerified((Put) mutation)) {
+ actual = mutation;
+ break;
+ }
+ }
+ if (actual == null) {
+ return;
+ }
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ return;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
+ }
+ Collections.sort(actualMutationList, MUTATION_TS_DESC_COMPARATOR);
+ }
+
+ private void cleanUpActualMutationList(List<Mutation> actualMutationList)
+ throws IOException {
+ Iterator<Mutation> iterator = actualMutationList.iterator();
+ Mutation previous = null;
+ while (iterator.hasNext()) {
+ Mutation mutation = iterator.next();
+ if ((mutation instanceof Put && !isVerified((Put) mutation)) ||
+ (mutation instanceof Delete && isDeleteFamilyVersion(mutation))) {
+ iterator.remove();
+ } else {
+ if (previous != null && getTimestamp(previous) == getTimestamp(mutation) &&
+ ((previous instanceof Put && mutation instanceof Put) ||
+ previous instanceof Delete && mutation instanceof Delete)) {
+ iterator.remove();
+ } else {
+ previous = mutation;
+ }
+ }
+ }
+ }
+
/**
- * indexRow is the set of all cells of all the row version of an index row from the index table. These are actual
- * cells. We group these cells based on timestamp and type (put vs delete), and form the actual set of
- * index mutations. indexKeyToMutationMap is a map from an index row key to a set of mutations that are generated
- * using the rebuild process (i.e., by replaying raw data table mutations). These sets are sets of expected
- * index mutations, one set for each index row key. Since not all mutations in the index table have both phase
- * (i.e., pre and post data phase) mutations, we cannot compare actual index mutations with expected one by one
- * and expect to find them identical. We need to consider concurrent data mutation effects, data table row write
- * failures, post index write failures. Thus, we need to allow some expected and actual mutations to be skipped
- * during comparing actual mutations to index mutations.
+ * There are two types of verification: without repair and with repair. Without-repair verification is done before
+ * or after index rebuild. It is done before index rebuild to identify the rows to be rebuilt. It is done after
+ * index rebuild to verify the rows that have been rebuilt. With-repair verification can be done anytime using
+ * the “-v ONLY” option to check the consistency of the index table.
+ *
+ * Unverified Rows
+ *
+ * For each mutable data table mutation during regular data table updates, two operations are done on the data table.
+ * One is to read the existing row state, and the second is to update the data table for this row. The processing of
+ * concurrent data mutations are serialized once for reading the existing row states, and then serialized again
+ * for updating the data table. In other words, they go through locking twice, i.e., [lock, read, unlock] and
+ * [lock, write, unlock]. Because of this two phase locking, for a pair of concurrent mutations (for the same row),
+ * the same row state can be read from the data table. This means the same existing index row can be made unverified
+ * twice with different timestamps, one for each concurrent mutation. These unverified mutations can be repaired
+ * from the data table later during HBase scans using the index read repair process. This is one of the reasons
+ * for having extra unverified rows in the index table. The other reason is the data table write failures.
+ * When a data table write fails, it leaves an unverified index row behind. These rows are never returned to clients,
+ * instead they are repaired, which means either they are rebuilt from their data table rows or they are deleted if
+ * their data table rows do not exist.
+ *
+ * Delete Family Version Markers
+ *
+ * The family version delete markers are generated by the read repair to remove extra unverified rows. They only
Review comment:
delete marks is not only generated by the read repair. If there is a column delete on the indexed data, there will be family version delete marker as well. Will it be a problem?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] swaroopak commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
swaroopak commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r398126994
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // find the first (latest) actual unverified put mutation
+ Mutation actual = null;
+ for (Mutation mutation : actualMutationList) {
+ if (mutation instanceof Put && !isVerified((Put) mutation)) {
+ actual = mutation;
+ break;
+ }
+ }
+ if (actual == null) {
+ return;
+ }
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ return;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
+ }
+ Collections.sort(actualMutationList, MUTATION_TS_DESC_COMPARATOR);
+ }
+
+ private void cleanUpActualMutationList(List<Mutation> actualMutationList)
+ throws IOException {
+ Iterator<Mutation> iterator = actualMutationList.iterator();
+ Mutation previous = null;
+ while (iterator.hasNext()) {
+ Mutation mutation = iterator.next();
+ if ((mutation instanceof Put && !isVerified((Put) mutation)) ||
+ (mutation instanceof Delete && isDeleteFamilyVersion(mutation))) {
+ iterator.remove();
+ } else {
+ if (previous != null && getTimestamp(previous) == getTimestamp(mutation) &&
+ ((previous instanceof Put && mutation instanceof Put) ||
+ previous instanceof Delete && mutation instanceof Delete)) {
+ iterator.remove();
+ } else {
+ previous = mutation;
+ }
+ }
+ }
+ }
+
/**
- * indexRow is the set of all cells of all the row version of an index row from the index table. These are actual
- * cells. We group these cells based on timestamp and type (put vs delete), and form the actual set of
- * index mutations. indexKeyToMutationMap is a map from an index row key to a set of mutations that are generated
- * using the rebuild process (i.e., by replaying raw data table mutations). These sets are sets of expected
- * index mutations, one set for each index row key. Since not all mutations in the index table have both phase
- * (i.e., pre and post data phase) mutations, we cannot compare actual index mutations with expected one by one
- * and expect to find them identical. We need to consider concurrent data mutation effects, data table row write
- * failures, post index write failures. Thus, we need to allow some expected and actual mutations to be skipped
- * during comparing actual mutations to index mutations.
+ * There are two types of verification: without repair and with repair. Without-repair verification is done before
+ * or after index rebuild. It is done before index rebuild to identify the rows to be rebuilt. It is done after
+ * index rebuild to verify the rows that have been rebuilt. With-repair verification can be done anytime using
+ * the “-v ONLY” option to check the consistency of the index table.
+ *
+ * Unverified Rows
+ *
+ * For each mutable data table mutation during regular data table updates, two operations are done on the data table.
+ * One is to read the existing row state, and the second is to update the data table for this row. The processing of
+ * concurrent data mutations are serialized once for reading the existing row states, and then serialized again
+ * for updating the data table. In other words, they go through locking twice, i.e., [lock, read, unlock] and
+ * [lock, write, unlock]. Because of this two phase locking, for a pair of concurrent mutations (for the same row),
+ * the same row state can be read from the data table. This means the same existing index row can be made unverified
+ * twice with different timestamps, one for each concurrent mutation. These unverified mutations can be repaired
+ * from the data table later during HBase scans using the index read repair process. This is one of the reasons
+ * for having extra unverified rows in the index table. The other reason is the data table write failures.
+ * When a data table write fails, it leaves an unverified index row behind. These rows are never returned to clients,
+ * instead they are repaired, which means either they are rebuilt from their data table rows or they are deleted if
+ * their data table rows do not exist.
+ *
+ * Delete Family Version Markers
+ *
+ * The family version delete markers are generated by the read repair to remove extra unverified rows. They only
Review comment:
I don't think that would be a problem. Because the data row would have already been updated (as index row has a delete marker that means data row delete i.e. the second phase was successful) and the expected index list would be built on that data row.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r397521193
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // find the first (latest) actual unverified put mutation
+ Mutation actual = null;
+ for (Mutation mutation : actualMutationList) {
+ if (mutation instanceof Put && !isVerified((Put) mutation)) {
+ actual = mutation;
+ break;
+ }
+ }
+ if (actual == null) {
+ return;
+ }
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ return;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
+ }
+ Collections.sort(actualMutationList, MUTATION_TS_DESC_COMPARATOR);
+ }
+
+ private void cleanUpActualMutationList(List<Mutation> actualMutationList)
+ throws IOException {
+ Iterator<Mutation> iterator = actualMutationList.iterator();
+ Mutation previous = null;
+ while (iterator.hasNext()) {
+ Mutation mutation = iterator.next();
+ if ((mutation instanceof Put && !isVerified((Put) mutation)) ||
+ (mutation instanceof Delete && isDeleteFamilyVersion(mutation))) {
+ iterator.remove();
+ } else {
+ if (previous != null && getTimestamp(previous) == getTimestamp(mutation) &&
+ ((previous instanceof Put && mutation instanceof Put) ||
+ previous instanceof Delete && mutation instanceof Delete)) {
+ iterator.remove();
+ } else {
+ previous = mutation;
+ }
+ }
+ }
+ }
+
/**
- * indexRow is the set of all cells of all the row version of an index row from the index table. These are actual
- * cells. We group these cells based on timestamp and type (put vs delete), and form the actual set of
- * index mutations. indexKeyToMutationMap is a map from an index row key to a set of mutations that are generated
- * using the rebuild process (i.e., by replaying raw data table mutations). These sets are sets of expected
- * index mutations, one set for each index row key. Since not all mutations in the index table have both phase
- * (i.e., pre and post data phase) mutations, we cannot compare actual index mutations with expected one by one
- * and expect to find them identical. We need to consider concurrent data mutation effects, data table row write
- * failures, post index write failures. Thus, we need to allow some expected and actual mutations to be skipped
- * during comparing actual mutations to index mutations.
+ * There are two types of verification: without repair and with repair. Without-repair verification is done before
+ * or after index rebuild. It is done before index rebuild to identify the rows to be rebuilt. It is done after
+ * index rebuild to verify the rows that have been rebuilt. With-repair verification can be done anytime using
+ * the “-v ONLY” option to check the consistency of the index table.
+ *
+ * Unverified Rows
+ *
+ * For each mutable data table mutation during regular data table updates, two operations are done on the data table.
+ * One is to read the existing row state, and the second is to update the data table for this row. The processing of
+ * concurrent data mutations are serialized once for reading the existing row states, and then serialized again
+ * for updating the data table. In other words, they go through locking twice, i.e., [lock, read, unlock] and
+ * [lock, write, unlock]. Because of this two phase locking, for a pair of concurrent mutations (for the same row),
+ * the same row state can be read from the data table. This means the same existing index row can be made unverified
+ * twice with different timestamps, one for each concurrent mutation. These unverified mutations can be repaired
+ * from the data table later during HBase scans using the index read repair process. This is one of the reasons
+ * for having extra unverified rows in the index table. The other reason is the data table write failures.
+ * When a data table write fails, it leaves an unverified index row behind. These rows are never returned to clients,
+ * instead they are repaired, which means either they are rebuilt from their data table rows or they are deleted if
+ * their data table rows do not exist.
+ *
+ * Delete Family Version Markers
+ *
+ * The family version delete markers are generated by the read repair to remove extra unverified rows. They only
Review comment:
delete marks is not only generated by the read repair. If there is a column delete on the indexed data, there will be family version delete marker as well.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] gjacoby126 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r398803618
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -613,39 +605,137 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
}
return getMutationsWithSameTS(put, del);
}
+ /**
+ * In this method, the actual list is repaired in memory using the expected list which is actually the output of
+ * rebuilding the index table row. The result of this repair is used only for verification.
+ */
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // Find the first (latest) actual unverified put mutation
+ List<Mutation> repairedMutationList = new ArrayList<>(expectedMutationList.size());
+ for (Mutation actual : actualMutationList) {
+ if (actual instanceof Put && !isVerified((Put) actual)) {
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ continue;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ repairedMutationList.add(mutation);
+ }
+ // Since we repair the entire history, there is no need to more than once
+ break;
+ }
+ }
+ if (repairedMutationList.isEmpty()) {
+ return;
+ }
+ actualMutationList.addAll(repairedMutationList);
+ Collections.sort(actualMutationList, MUTATION_TS_DESC_COMPARATOR);
+ }
+
+ private void cleanUpActualMutationList(List<Mutation> actualMutationList)
+ throws IOException {
+ Iterator<Mutation> iterator = actualMutationList.iterator();
+ Mutation previous = null;
+ while (iterator.hasNext()) {
+ Mutation mutation = iterator.next();
+ if ((mutation instanceof Put && !isVerified((Put) mutation)) ||
+ (mutation instanceof Delete && isDeleteFamilyVersion(mutation))) {
+ iterator.remove();
+ } else {
+ if (previous != null && getTimestamp(previous) == getTimestamp(mutation) &&
+ ((previous instanceof Put && mutation instanceof Put) ||
+ previous instanceof Delete && mutation instanceof Delete)) {
+ iterator.remove();
+ } else {
+ previous = mutation;
+ }
+ }
+ }
+ }
/**
- * indexRow is the set of all cells of all the row version of an index row from the index table. These are actual
- * cells. We group these cells based on timestamp and type (put vs delete), and form the actual set of
- * index mutations. indexKeyToMutationMap is a map from an index row key to a set of mutations that are generated
- * using the rebuild process (i.e., by replaying raw data table mutations). These sets are sets of expected
- * index mutations, one set for each index row key. Since not all mutations in the index table have both phase
- * (i.e., pre and post data phase) mutations, we cannot compare actual index mutations with expected one by one
- * and expect to find them identical. We need to consider concurrent data mutation effects, data table row write
- * failures, post index write failures. Thus, we need to allow some expected and actual mutations to be skipped
- * during comparing actual mutations to index mutations.
+ * There are two types of verification: without repair and with repair. Without-repair verification is done before
+ * or after index rebuild. It is done before index rebuild to identify the rows to be rebuilt. It is done after
+ * index rebuild to verify the rows that have been rebuilt. With-repair verification can be done anytime using
+ * the “-v ONLY” option to check the consistency of the index table.
Review comment:
Otherwise a very nice explanation
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r397551472
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
Review comment:
I am just thinking aloud here. Read repair has similar logic to generate the expected index mutation, but only for specific timestamp. Is it possible to leverage the code there or unify them?
One side question. From the code of repairIndexRows, it looks like opening a lot of scan. If there are multiple concurrent read repair happen (may be caused by some bug int the future or network issue), will it overload the hbase region server and cause the cluster not be able to recover from such network hitcup?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] kadirozde closed pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
kadirozde closed pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] kadirozde commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r399513548
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -613,39 +605,137 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
}
return getMutationsWithSameTS(put, del);
}
+ /**
+ * In this method, the actual list is repaired in memory using the expected list which is actually the output of
+ * rebuilding the index table row. The result of this repair is used only for verification.
+ */
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // Find the first (latest) actual unverified put mutation
+ List<Mutation> repairedMutationList = new ArrayList<>(expectedMutationList.size());
+ for (Mutation actual : actualMutationList) {
+ if (actual instanceof Put && !isVerified((Put) actual)) {
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ continue;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ repairedMutationList.add(mutation);
+ }
+ // Since we repair the entire history, there is no need to more than once
+ break;
+ }
+ }
+ if (repairedMutationList.isEmpty()) {
+ return;
+ }
+ actualMutationList.addAll(repairedMutationList);
+ Collections.sort(actualMutationList, MUTATION_TS_DESC_COMPARATOR);
+ }
+
+ private void cleanUpActualMutationList(List<Mutation> actualMutationList)
+ throws IOException {
+ Iterator<Mutation> iterator = actualMutationList.iterator();
+ Mutation previous = null;
+ while (iterator.hasNext()) {
+ Mutation mutation = iterator.next();
+ if ((mutation instanceof Put && !isVerified((Put) mutation)) ||
+ (mutation instanceof Delete && isDeleteFamilyVersion(mutation))) {
+ iterator.remove();
+ } else {
+ if (previous != null && getTimestamp(previous) == getTimestamp(mutation) &&
+ ((previous instanceof Put && mutation instanceof Put) ||
+ previous instanceof Delete && mutation instanceof Delete)) {
+ iterator.remove();
+ } else {
+ previous = mutation;
+ }
+ }
+ }
+ }
/**
- * indexRow is the set of all cells of all the row version of an index row from the index table. These are actual
- * cells. We group these cells based on timestamp and type (put vs delete), and form the actual set of
- * index mutations. indexKeyToMutationMap is a map from an index row key to a set of mutations that are generated
- * using the rebuild process (i.e., by replaying raw data table mutations). These sets are sets of expected
- * index mutations, one set for each index row key. Since not all mutations in the index table have both phase
- * (i.e., pre and post data phase) mutations, we cannot compare actual index mutations with expected one by one
- * and expect to find them identical. We need to consider concurrent data mutation effects, data table row write
- * failures, post index write failures. Thus, we need to allow some expected and actual mutations to be skipped
- * during comparing actual mutations to index mutations.
+ * There are two types of verification: without repair and with repair. Without-repair verification is done before
+ * or after index rebuild. It is done before index rebuild to identify the rows to be rebuilt. It is done after
+ * index rebuild to verify the rows that have been rebuilt. With-repair verification can be done anytime using
+ * the “-v ONLY” option to check the consistency of the index table.
Review comment:
I will add the suggest comment
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] kadirozde commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r398346481
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // find the first (latest) actual unverified put mutation
+ Mutation actual = null;
+ for (Mutation mutation : actualMutationList) {
+ if (mutation instanceof Put && !isVerified((Put) mutation)) {
+ actual = mutation;
+ break;
+ }
+ }
+ if (actual == null) {
+ return;
+ }
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ return;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
+ }
+ Collections.sort(actualMutationList, MUTATION_TS_DESC_COMPARATOR);
+ }
+
+ private void cleanUpActualMutationList(List<Mutation> actualMutationList)
Review comment:
@wangweiming800, your understanding of mutation types is correct. However, I have no idea what replaying an index mutation means. Let's discuss this offline.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] gjacoby126 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r398806437
##########
File path: phoenix-core/src/test/java/org/apache/phoenix/index/VerifySingleIndexRowTest.java
##########
@@ -350,6 +351,7 @@ public void testVerifySingleIndexRow_allUnverified() throws IOException {
}
}
+ @Ignore
Review comment:
Why are we ignoring tests rather than removing or fixing them?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r398921503
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // find the first (latest) actual unverified put mutation
+ Mutation actual = null;
+ for (Mutation mutation : actualMutationList) {
+ if (mutation instanceof Put && !isVerified((Put) mutation)) {
+ actual = mutation;
+ break;
+ }
+ }
+ if (actual == null) {
+ return;
+ }
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ return;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
+ }
+ Collections.sort(actualMutationList, MUTATION_TS_DESC_COMPARATOR);
+ }
+
+ private void cleanUpActualMutationList(List<Mutation> actualMutationList)
+ throws IOException {
+ Iterator<Mutation> iterator = actualMutationList.iterator();
+ Mutation previous = null;
+ while (iterator.hasNext()) {
+ Mutation mutation = iterator.next();
+ if ((mutation instanceof Put && !isVerified((Put) mutation)) ||
+ (mutation instanceof Delete && isDeleteFamilyVersion(mutation))) {
+ iterator.remove();
Review comment:
@kadirozde , I am wondering whether it will cause false alarm when the last data mutation is put and fails on the 3rd phase and there is no read repair to fix this unverified row before inline validation. In this case, the unverified index row will be included in the expected mutation list, but cannot find in the actual mutation list.
Following is the link I come up a scenario to simulate this case. We may discuss it offline
https://docs.google.com/spreadsheets/d/1FDKkcGrJobR5Jtd73vNSVfC1yDDrisyuqUdYWLde5rw/edit?usp=sharing
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] kadirozde commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r398227615
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
Review comment:
This simulates the read repair given that the list of revisions for the actual and the expected (repaired form) mutations. It is much simpler than the read repair process in GlobalIndexChecker which works on one index row at a time and leverages the HBase scans on the actual table, which is not the case here. Also, GlobalIndexChecker also purges the invalid rows from HBase in addition rebuilding unverified rows. So, they are very different in nature and not sure how they can be unified.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] kadirozde commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r398230498
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // find the first (latest) actual unverified put mutation
+ Mutation actual = null;
+ for (Mutation mutation : actualMutationList) {
+ if (mutation instanceof Put && !isVerified((Put) mutation)) {
+ actual = mutation;
+ break;
+ }
+ }
+ if (actual == null) {
+ return;
+ }
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ return;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
+ }
+ Collections.sort(actualMutationList, MUTATION_TS_DESC_COMPARATOR);
+ }
+
+ private void cleanUpActualMutationList(List<Mutation> actualMutationList)
+ throws IOException {
+ Iterator<Mutation> iterator = actualMutationList.iterator();
+ Mutation previous = null;
+ while (iterator.hasNext()) {
+ Mutation mutation = iterator.next();
+ if ((mutation instanceof Put && !isVerified((Put) mutation)) ||
+ (mutation instanceof Delete && isDeleteFamilyVersion(mutation))) {
+ iterator.remove();
+ } else {
+ if (previous != null && getTimestamp(previous) == getTimestamp(mutation) &&
+ ((previous instanceof Put && mutation instanceof Put) ||
+ previous instanceof Delete && mutation instanceof Delete)) {
+ iterator.remove();
+ } else {
+ previous = mutation;
+ }
+ }
+ }
+ }
+
/**
- * indexRow is the set of all cells of all the row version of an index row from the index table. These are actual
- * cells. We group these cells based on timestamp and type (put vs delete), and form the actual set of
- * index mutations. indexKeyToMutationMap is a map from an index row key to a set of mutations that are generated
- * using the rebuild process (i.e., by replaying raw data table mutations). These sets are sets of expected
- * index mutations, one set for each index row key. Since not all mutations in the index table have both phase
- * (i.e., pre and post data phase) mutations, we cannot compare actual index mutations with expected one by one
- * and expect to find them identical. We need to consider concurrent data mutation effects, data table row write
- * failures, post index write failures. Thus, we need to allow some expected and actual mutations to be skipped
- * during comparing actual mutations to index mutations.
+ * There are two types of verification: without repair and with repair. Without-repair verification is done before
+ * or after index rebuild. It is done before index rebuild to identify the rows to be rebuilt. It is done after
+ * index rebuild to verify the rows that have been rebuilt. With-repair verification can be done anytime using
+ * the “-v ONLY” option to check the consistency of the index table.
+ *
+ * Unverified Rows
+ *
+ * For each mutable data table mutation during regular data table updates, two operations are done on the data table.
+ * One is to read the existing row state, and the second is to update the data table for this row. The processing of
+ * concurrent data mutations are serialized once for reading the existing row states, and then serialized again
+ * for updating the data table. In other words, they go through locking twice, i.e., [lock, read, unlock] and
+ * [lock, write, unlock]. Because of this two phase locking, for a pair of concurrent mutations (for the same row),
+ * the same row state can be read from the data table. This means the same existing index row can be made unverified
+ * twice with different timestamps, one for each concurrent mutation. These unverified mutations can be repaired
+ * from the data table later during HBase scans using the index read repair process. This is one of the reasons
+ * for having extra unverified rows in the index table. The other reason is the data table write failures.
+ * When a data table write fails, it leaves an unverified index row behind. These rows are never returned to clients,
+ * instead they are repaired, which means either they are rebuilt from their data table rows or they are deleted if
+ * their data table rows do not exist.
+ *
+ * Delete Family Version Markers
+ *
+ * The family version delete markers are generated by the read repair to remove extra unverified rows. They only
Review comment:
@wangweiming800, "If there is a column delete on the indexed column in data table, there will be family version delete marker as well" is not correct. We do not convert delete column markers to delete family markers. Actually, delete column markers translate to missing columns in the index row mutations.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] gjacoby126 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r398801976
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -613,39 +605,137 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
}
return getMutationsWithSameTS(put, del);
}
+ /**
+ * In this method, the actual list is repaired in memory using the expected list which is actually the output of
+ * rebuilding the index table row. The result of this repair is used only for verification.
+ */
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // Find the first (latest) actual unverified put mutation
+ List<Mutation> repairedMutationList = new ArrayList<>(expectedMutationList.size());
+ for (Mutation actual : actualMutationList) {
+ if (actual instanceof Put && !isVerified((Put) actual)) {
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ continue;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ repairedMutationList.add(mutation);
+ }
+ // Since we repair the entire history, there is no need to more than once
+ break;
+ }
+ }
+ if (repairedMutationList.isEmpty()) {
+ return;
+ }
+ actualMutationList.addAll(repairedMutationList);
+ Collections.sort(actualMutationList, MUTATION_TS_DESC_COMPARATOR);
+ }
+
+ private void cleanUpActualMutationList(List<Mutation> actualMutationList)
+ throws IOException {
+ Iterator<Mutation> iterator = actualMutationList.iterator();
+ Mutation previous = null;
+ while (iterator.hasNext()) {
+ Mutation mutation = iterator.next();
+ if ((mutation instanceof Put && !isVerified((Put) mutation)) ||
+ (mutation instanceof Delete && isDeleteFamilyVersion(mutation))) {
+ iterator.remove();
+ } else {
+ if (previous != null && getTimestamp(previous) == getTimestamp(mutation) &&
+ ((previous instanceof Put && mutation instanceof Put) ||
+ previous instanceof Delete && mutation instanceof Delete)) {
+ iterator.remove();
+ } else {
+ previous = mutation;
+ }
+ }
+ }
+ }
/**
- * indexRow is the set of all cells of all the row version of an index row from the index table. These are actual
- * cells. We group these cells based on timestamp and type (put vs delete), and form the actual set of
- * index mutations. indexKeyToMutationMap is a map from an index row key to a set of mutations that are generated
- * using the rebuild process (i.e., by replaying raw data table mutations). These sets are sets of expected
- * index mutations, one set for each index row key. Since not all mutations in the index table have both phase
- * (i.e., pre and post data phase) mutations, we cannot compare actual index mutations with expected one by one
- * and expect to find them identical. We need to consider concurrent data mutation effects, data table row write
- * failures, post index write failures. Thus, we need to allow some expected and actual mutations to be skipped
- * during comparing actual mutations to index mutations.
+ * There are two types of verification: without repair and with repair. Without-repair verification is done before
+ * or after index rebuild. It is done before index rebuild to identify the rows to be rebuilt. It is done after
+ * index rebuild to verify the rows that have been rebuilt. With-repair verification can be done anytime using
+ * the “-v ONLY” option to check the consistency of the index table.
Review comment:
"Note that with-repair verification simulates read repair in-memory for the purpose of verification, but does not actually repair the data in the index."
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r398332277
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // find the first (latest) actual unverified put mutation
+ Mutation actual = null;
+ for (Mutation mutation : actualMutationList) {
+ if (mutation instanceof Put && !isVerified((Put) mutation)) {
+ actual = mutation;
+ break;
+ }
+ }
+ if (actual == null) {
+ return;
+ }
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ return;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
+ }
+ Collections.sort(actualMutationList, MUTATION_TS_DESC_COMPARATOR);
+ }
+
+ private void cleanUpActualMutationList(List<Mutation> actualMutationList)
Review comment:
@kadirozde , I am thinking whether it is possible to replay the actual mutations by ignoring the unverified mutations and compare them with the expected mutations.
If I understand correctly, there are only 4 types of mutations as a result of raw scan on index table.
unverified put: it is introduced by failure on 2nd or 3rd phase data mutation, successful data row deletion or ongoing data mutation.
verified put: it is introduced by successful data put mutation or read repair
deleteFamily: it is introduced by successful data mutation which leads to the change or remove of the index row key.
deleteFamilyVersion: it is introduced by the read repair.
Since verified put has the full information of the index mutation, even it is generated by the read repair. So if we replay all these 3 kinds of mutations by the time order, we are expected to get the expected mutation list.
Do I misunderstand something 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
With regards,
Apache Git Services
[GitHub] [phoenix] wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r397521193
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // find the first (latest) actual unverified put mutation
+ Mutation actual = null;
+ for (Mutation mutation : actualMutationList) {
+ if (mutation instanceof Put && !isVerified((Put) mutation)) {
+ actual = mutation;
+ break;
+ }
+ }
+ if (actual == null) {
+ return;
+ }
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ return;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
+ }
+ Collections.sort(actualMutationList, MUTATION_TS_DESC_COMPARATOR);
+ }
+
+ private void cleanUpActualMutationList(List<Mutation> actualMutationList)
+ throws IOException {
+ Iterator<Mutation> iterator = actualMutationList.iterator();
+ Mutation previous = null;
+ while (iterator.hasNext()) {
+ Mutation mutation = iterator.next();
+ if ((mutation instanceof Put && !isVerified((Put) mutation)) ||
+ (mutation instanceof Delete && isDeleteFamilyVersion(mutation))) {
+ iterator.remove();
+ } else {
+ if (previous != null && getTimestamp(previous) == getTimestamp(mutation) &&
+ ((previous instanceof Put && mutation instanceof Put) ||
+ previous instanceof Delete && mutation instanceof Delete)) {
+ iterator.remove();
+ } else {
+ previous = mutation;
+ }
+ }
+ }
+ }
+
/**
- * indexRow is the set of all cells of all the row version of an index row from the index table. These are actual
- * cells. We group these cells based on timestamp and type (put vs delete), and form the actual set of
- * index mutations. indexKeyToMutationMap is a map from an index row key to a set of mutations that are generated
- * using the rebuild process (i.e., by replaying raw data table mutations). These sets are sets of expected
- * index mutations, one set for each index row key. Since not all mutations in the index table have both phase
- * (i.e., pre and post data phase) mutations, we cannot compare actual index mutations with expected one by one
- * and expect to find them identical. We need to consider concurrent data mutation effects, data table row write
- * failures, post index write failures. Thus, we need to allow some expected and actual mutations to be skipped
- * during comparing actual mutations to index mutations.
+ * There are two types of verification: without repair and with repair. Without-repair verification is done before
+ * or after index rebuild. It is done before index rebuild to identify the rows to be rebuilt. It is done after
+ * index rebuild to verify the rows that have been rebuilt. With-repair verification can be done anytime using
+ * the “-v ONLY” option to check the consistency of the index table.
+ *
+ * Unverified Rows
+ *
+ * For each mutable data table mutation during regular data table updates, two operations are done on the data table.
+ * One is to read the existing row state, and the second is to update the data table for this row. The processing of
+ * concurrent data mutations are serialized once for reading the existing row states, and then serialized again
+ * for updating the data table. In other words, they go through locking twice, i.e., [lock, read, unlock] and
+ * [lock, write, unlock]. Because of this two phase locking, for a pair of concurrent mutations (for the same row),
+ * the same row state can be read from the data table. This means the same existing index row can be made unverified
+ * twice with different timestamps, one for each concurrent mutation. These unverified mutations can be repaired
+ * from the data table later during HBase scans using the index read repair process. This is one of the reasons
+ * for having extra unverified rows in the index table. The other reason is the data table write failures.
+ * When a data table write fails, it leaves an unverified index row behind. These rows are never returned to clients,
+ * instead they are repaired, which means either they are rebuilt from their data table rows or they are deleted if
+ * their data table rows do not exist.
+ *
+ * Delete Family Version Markers
+ *
+ * The family version delete markers are generated by the read repair to remove extra unverified rows. They only
Review comment:
delete marks is not only generated by the read repair. If there is a column delete on the indexed data, there will be family version delete marker as well. Will it be a problem?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] kadirozde commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r399522286
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // find the first (latest) actual unverified put mutation
+ Mutation actual = null;
+ for (Mutation mutation : actualMutationList) {
+ if (mutation instanceof Put && !isVerified((Put) mutation)) {
+ actual = mutation;
+ break;
+ }
+ }
+ if (actual == null) {
+ return;
+ }
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ return;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
+ }
+ Collections.sort(actualMutationList, MUTATION_TS_DESC_COMPARATOR);
+ }
+
+ private void cleanUpActualMutationList(List<Mutation> actualMutationList)
+ throws IOException {
+ Iterator<Mutation> iterator = actualMutationList.iterator();
+ Mutation previous = null;
+ while (iterator.hasNext()) {
+ Mutation mutation = iterator.next();
+ if ((mutation instanceof Put && !isVerified((Put) mutation)) ||
+ (mutation instanceof Delete && isDeleteFamilyVersion(mutation))) {
+ iterator.remove();
Review comment:
@wangweiming800 and I discussed and decided to resolve this conversation
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r398921503
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // find the first (latest) actual unverified put mutation
+ Mutation actual = null;
+ for (Mutation mutation : actualMutationList) {
+ if (mutation instanceof Put && !isVerified((Put) mutation)) {
+ actual = mutation;
+ break;
+ }
+ }
+ if (actual == null) {
+ return;
+ }
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ return;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
+ }
+ Collections.sort(actualMutationList, MUTATION_TS_DESC_COMPARATOR);
+ }
+
+ private void cleanUpActualMutationList(List<Mutation> actualMutationList)
+ throws IOException {
+ Iterator<Mutation> iterator = actualMutationList.iterator();
+ Mutation previous = null;
+ while (iterator.hasNext()) {
+ Mutation mutation = iterator.next();
+ if ((mutation instanceof Put && !isVerified((Put) mutation)) ||
+ (mutation instanceof Delete && isDeleteFamilyVersion(mutation))) {
+ iterator.remove();
Review comment:
I am wondering whether it will cause false alarm when the last data mutation is put and fails on the 3rd phase and there is no read repair to fix this unverified row before inline validation. In this case, the unverified index row will be included in the expected mutation list, but cannot find in the actual mutation list.
Following is the link I come up a scenario to simulate this case. We may discuss it offline
https://docs.google.com/spreadsheets/d/1FDKkcGrJobR5Jtd73vNSVfC1yDDrisyuqUdYWLde5rw/edit?usp=sharing
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r398250136
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // find the first (latest) actual unverified put mutation
+ Mutation actual = null;
+ for (Mutation mutation : actualMutationList) {
+ if (mutation instanceof Put && !isVerified((Put) mutation)) {
+ actual = mutation;
+ break;
+ }
+ }
+ if (actual == null) {
+ return;
+ }
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ return;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
+ }
+ Collections.sort(actualMutationList, MUTATION_TS_DESC_COMPARATOR);
+ }
+
+ private void cleanUpActualMutationList(List<Mutation> actualMutationList)
+ throws IOException {
+ Iterator<Mutation> iterator = actualMutationList.iterator();
+ Mutation previous = null;
+ while (iterator.hasNext()) {
+ Mutation mutation = iterator.next();
+ if ((mutation instanceof Put && !isVerified((Put) mutation)) ||
+ (mutation instanceof Delete && isDeleteFamilyVersion(mutation))) {
+ iterator.remove();
+ } else {
+ if (previous != null && getTimestamp(previous) == getTimestamp(mutation) &&
+ ((previous instanceof Put && mutation instanceof Put) ||
+ previous instanceof Delete && mutation instanceof Delete)) {
+ iterator.remove();
+ } else {
+ previous = mutation;
+ }
+ }
+ }
+ }
+
/**
- * indexRow is the set of all cells of all the row version of an index row from the index table. These are actual
- * cells. We group these cells based on timestamp and type (put vs delete), and form the actual set of
- * index mutations. indexKeyToMutationMap is a map from an index row key to a set of mutations that are generated
- * using the rebuild process (i.e., by replaying raw data table mutations). These sets are sets of expected
- * index mutations, one set for each index row key. Since not all mutations in the index table have both phase
- * (i.e., pre and post data phase) mutations, we cannot compare actual index mutations with expected one by one
- * and expect to find them identical. We need to consider concurrent data mutation effects, data table row write
- * failures, post index write failures. Thus, we need to allow some expected and actual mutations to be skipped
- * during comparing actual mutations to index mutations.
+ * There are two types of verification: without repair and with repair. Without-repair verification is done before
+ * or after index rebuild. It is done before index rebuild to identify the rows to be rebuilt. It is done after
+ * index rebuild to verify the rows that have been rebuilt. With-repair verification can be done anytime using
+ * the “-v ONLY” option to check the consistency of the index table.
+ *
+ * Unverified Rows
+ *
+ * For each mutable data table mutation during regular data table updates, two operations are done on the data table.
+ * One is to read the existing row state, and the second is to update the data table for this row. The processing of
+ * concurrent data mutations are serialized once for reading the existing row states, and then serialized again
+ * for updating the data table. In other words, they go through locking twice, i.e., [lock, read, unlock] and
+ * [lock, write, unlock]. Because of this two phase locking, for a pair of concurrent mutations (for the same row),
+ * the same row state can be read from the data table. This means the same existing index row can be made unverified
+ * twice with different timestamps, one for each concurrent mutation. These unverified mutations can be repaired
+ * from the data table later during HBase scans using the index read repair process. This is one of the reasons
+ * for having extra unverified rows in the index table. The other reason is the data table write failures.
+ * When a data table write fails, it leaves an unverified index row behind. These rows are never returned to clients,
+ * instead they are repaired, which means either they are rebuilt from their data table rows or they are deleted if
+ * their data table rows do not exist.
+ *
+ * Delete Family Version Markers
+ *
+ * The family version delete markers are generated by the read repair to remove extra unverified rows. They only
Review comment:
thanks for explanation.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r398250816
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
Review comment:
thanks
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r397528808
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // find the first (latest) actual unverified put mutation
+ Mutation actual = null;
+ for (Mutation mutation : actualMutationList) {
+ if (mutation instanceof Put && !isVerified((Put) mutation)) {
+ actual = mutation;
+ break;
+ }
+ }
+ if (actual == null) {
+ return;
+ }
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ return;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
+ }
+ Collections.sort(actualMutationList, MUTATION_TS_DESC_COMPARATOR);
+ }
+
+ private void cleanUpActualMutationList(List<Mutation> actualMutationList)
Review comment:
Before this PR, I think it makes sense to ignore these mutations, as it only targets the ones with timestamp > expectedMutation.timestamp.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r398332277
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // find the first (latest) actual unverified put mutation
+ Mutation actual = null;
+ for (Mutation mutation : actualMutationList) {
+ if (mutation instanceof Put && !isVerified((Put) mutation)) {
+ actual = mutation;
+ break;
+ }
+ }
+ if (actual == null) {
+ return;
+ }
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ return;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
+ }
+ Collections.sort(actualMutationList, MUTATION_TS_DESC_COMPARATOR);
+ }
+
+ private void cleanUpActualMutationList(List<Mutation> actualMutationList)
Review comment:
@kadirozde , I am thinking whether it is possible to replay the actual mutations by ignoring the unverified mutations and compare them with the expected mutations.
If I understand correctly, there are only 4 types of mutations as a result of raw scan on index table.
unverified put: it is introduced by failure on 2nd or 3rd phase data mutation, successful data row deletion or ongoing data mutation.
verified put: it is introduced by successful data put mutation or read repair
deleteFamily: it is introduced by successful data mutation which leads to the change or remove of the index row key. T
deleteFamilyVersion: it is introduced by the read repair.
Since verified put has the full information of the index mutation, even it is generated by the read repair. So if we replay all these 3 kinds of mutations by the time order, we are expected to get the expected mutation list.
Do I misunderstand something 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
With regards,
Apache Git Services
[GitHub] [phoenix] wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
wangweiming800 commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r397551472
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
Review comment:
I am just thinking aloud here. Read repair has similar logic to generate the expected index mutation, but only for specific timestamp. Is it possible to leverage the code there or unify them?
One side question. From the code of repairIndexRows, it looks like opening a lot of scan. If there are multiple concurrent read repair happen (may be caused by some bug int the future or network issue), will it overload the hbase region server and cause the cluster not be able to recover from such network hiccup?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] kadirozde commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r398228255
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -614,38 +606,132 @@ private boolean isDeleteFamilyVersion(Mutation mutation) {
return getMutationsWithSameTS(put, del);
}
+ private void repairActualMutationList(List<Mutation> actualMutationList, List<Mutation> expectedMutationList)
+ throws IOException {
+ // find the first (latest) actual unverified put mutation
+ Mutation actual = null;
+ for (Mutation mutation : actualMutationList) {
+ if (mutation instanceof Put && !isVerified((Put) mutation)) {
+ actual = mutation;
+ break;
+ }
+ }
+ if (actual == null) {
+ return;
+ }
+ long ts = getTimestamp(actual);
+ int expectedIndex;
+ int expectedListSize = expectedMutationList.size();
+ for (expectedIndex = 0; expectedIndex < expectedListSize; expectedIndex++) {
+ if (getTimestamp(expectedMutationList.get(expectedIndex)) <= ts) {
+ if (expectedIndex > 0) {
+ expectedIndex--;
+ }
+ break;
+ }
+ }
+ if (expectedIndex == expectedListSize) {
+ return;
+ }
+ for (; expectedIndex < expectedListSize; expectedIndex++) {
+ Mutation mutation = expectedMutationList.get(expectedIndex);
+ if (mutation instanceof Put) {
+ mutation = new Put((Put) mutation);
+ } else {
+ mutation = new Delete((Delete) mutation);
+ }
+ actualMutationList.add(mutation);
+ }
+ Collections.sort(actualMutationList, MUTATION_TS_DESC_COMPARATOR);
+ }
+
+ private void cleanUpActualMutationList(List<Mutation> actualMutationList)
Review comment:
@wangweiming800, Please see the updated the code above and let me know if you have a question on this
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [phoenix] kadirozde commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #741: PHOENIX-5791 Eliminate false invalid row detection due to concurrent …
URL: https://github.com/apache/phoenix/pull/741#discussion_r399521579
##########
File path: phoenix-core/src/test/java/org/apache/phoenix/index/VerifySingleIndexRowTest.java
##########
@@ -350,6 +351,7 @@ public void testVerifySingleIndexRow_allUnverified() throws IOException {
}
}
+ @Ignore
Review comment:
@swaroopak and I worked together and fixed the failed unit tests. We needed to do some changes in the unit tests to adjust them for the changes of this PR. The unit tests caught couple issues in the code and we fixed them too. I now better appreciate this unit test :-)
----------------------------------------------------------------
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
With regards,
Apache Git Services