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/12/02 18:28:24 UTC

[GitHub] [phoenix] gokceni commented on a change in pull request #995: PHOENIX-6200 Add counters for extra index rows, log results to PIT and PIT_RESULT table

gokceni commented on a change in pull request #995:
URL: https://github.com/apache/phoenix/pull/995#discussion_r534357433



##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexRepairRegionScannerIT.java
##########
@@ -163,35 +308,251 @@ public void testRepairExtraIndexRows() throws Exception {
         String indexTableName = generateUniqueName();
         String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
         Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            initTablesAndAddExtraRowsToIndex(conn, schemaName, dataTableName, indexTableName, NROWS);
+
+            // do index rebuild without -fi and check with scrutiny that index tool failed to fix the extra rows
+            IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.BEFORE);
+
+            boolean failed;
+            try {
+                IndexScrutiny.scrutinizeIndex(conn, dataTableFullName, indexTableFullName);
+                failed = false;
+            } catch (AssertionError e) {
+                failed = true;
+            }
+            assertTrue(failed);
+
+            // now repair the index with -fi
+            IndexTool indexTool = IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.BEFORE, "-fi");
+
+            long actualRowCount = IndexScrutiny.scrutinizeIndex(conn, dataTableFullName, indexTableFullName);
+            assertEquals(NROWS, actualRowCount);
+
+            assertExtraCounters(indexTool, NROWS, 0, true);
+        }
+    }
+
+    @Test
+    public void testRepairExtraIndexRows_PostIndexUpdateFailure_overwrite() throws Exception {
+        if (!mutable) {
+            return;
+        }
+        final int NROWS = 4;
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
         try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
             conn.createStatement().execute("CREATE TABLE " + dataTableFullName
-                    + " (ID INTEGER NOT NULL PRIMARY KEY, VAL1 INTEGER, VAL2 INTEGER) "
-                    + tableDDLOptions);
+                + " (ID INTEGER NOT NULL PRIMARY KEY, VAL1 INTEGER, VAL2 INTEGER) " + tableDDLOptions);
+            conn.createStatement().execute(String.format(
+                "CREATE INDEX %s ON %s (VAL1) INCLUDE (VAL2)", indexTableName, dataTableFullName));
+
             PreparedStatement dataPreparedStatement =
-                    conn.prepareStatement("UPSERT INTO " + dataTableFullName + " VALUES(?,?,?)");
+                conn.prepareStatement("UPSERT INTO " + dataTableFullName + " VALUES(?,?,?)");
             for (int i = 1; i <= NROWS; i++) {
                 dataPreparedStatement.setInt(1, i);
                 dataPreparedStatement.setInt(2, i + 1);
                 dataPreparedStatement.setInt(3, i * 2);
                 dataPreparedStatement.execute();
             }
             conn.commit();
+
+            IndexRegionObserver.setFailPostIndexUpdatesForTesting(true);
+            conn.createStatement().execute("UPSERT INTO " + dataTableFullName + " VALUES(3, 100, 200)");
+            conn.commit();
+            IndexRegionObserver.setFailPostIndexUpdatesForTesting(false);
+
+            IndexTool indexTool = IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.BEFORE, "-fi");
+
+            CounterGroup mrJobCounters = IndexToolIT.getMRJobCounters(indexTool);
+            assertEquals(2,
+                mrJobCounters.findCounter(BEFORE_REBUILD_INVALID_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(2,
+                mrJobCounters.findCounter(BEFORE_REBUILD_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_VERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+
+            indexTool = IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.ONLY, "-fi");
+            mrJobCounters = IndexToolIT.getMRJobCounters(indexTool);
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REBUILD_INVALID_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REBUILD_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_VERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+
+            long actualRowCount = IndexScrutiny.scrutinizeIndex(conn, dataTableFullName, indexTableFullName);
+            assertEquals(NROWS, actualRowCount);
+        }
+    }
+
+    @Test
+    public void testRepairExtraIndexRows_PostIndexUpdateFailure_delete() throws Exception {
+        if (!mutable) {
+            return;
+        }
+        final int NROWS = 4;
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            conn.createStatement().execute("CREATE TABLE " + dataTableFullName
+                + " (ID INTEGER NOT NULL PRIMARY KEY, VAL1 INTEGER, VAL2 INTEGER) " + tableDDLOptions);
             conn.createStatement().execute(String.format(
-                    "CREATE INDEX %s ON %s (VAL1) INCLUDE (VAL2)", indexTableName, dataTableFullName));
-            // Add extra index rows
-            PreparedStatement indexPreparedStatement =
-                    conn.prepareStatement("UPSERT INTO " + indexTableFullName + " VALUES(?,?,?)");
+                "CREATE INDEX %s ON %s (VAL1) INCLUDE (VAL2)", indexTableName, dataTableFullName));
 
-            for (int i = NROWS + 1; i <= 2 * NROWS; i++) {
-                indexPreparedStatement.setInt(1, i + 1); // the indexed column
-                indexPreparedStatement.setInt(2, i); // the data pk column
-                indexPreparedStatement.setInt(3, i * 2); // the included column
-                indexPreparedStatement.execute();
+            PreparedStatement dataPreparedStatement =
+                conn.prepareStatement("UPSERT INTO " + dataTableFullName + " VALUES(?,?,?)");
+            for (int i = 1; i <= NROWS; i++) {
+                dataPreparedStatement.setInt(1, i);
+                dataPreparedStatement.setInt(2, i + 1);
+                dataPreparedStatement.setInt(3, i * 2);
+                dataPreparedStatement.execute();
             }
             conn.commit();
-            // Set all index row statuses to verified so that read verify will not fix them. We want them to be fixed
-            // by IndexRepairRegionScanner
-            setIndexRowStatusesToVerified(conn, dataTableFullName, indexTableFullName);
+
+            IndexRegionObserver.setFailPostIndexUpdatesForTesting(true);
+            conn.createStatement().execute("DELETE FROM " + dataTableFullName + " WHERE ID = 3");
+            conn.commit();
+            IndexRegionObserver.setFailPostIndexUpdatesForTesting(false);
+            TestUtil.doMajorCompaction(conn, dataTableFullName);
+
+            IndexTool indexTool = IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.BEFORE, "-fi");
+
+            CounterGroup mrJobCounters = IndexToolIT.getMRJobCounters(indexTool);
+
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REBUILD_INVALID_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REBUILD_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_VERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(1,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+
+            indexTool = IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.ONLY, "-fi");
+            mrJobCounters = IndexToolIT.getMRJobCounters(indexTool);
+
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REBUILD_INVALID_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REBUILD_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_VERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+
+            long actualRowCount = IndexScrutiny.scrutinizeIndex(conn, dataTableFullName, indexTableFullName);
+            assertEquals(NROWS - 1, actualRowCount);
+        }
+    }
+
+    @Test
+    public void testRepairExtraIndexRows_DataTableUpdateFailure() throws Exception {
+        if (!mutable) {
+            return;
+        }
+        final int NROWS = 20;
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            conn.createStatement().execute("CREATE TABLE " + dataTableFullName
+                + " (ID INTEGER NOT NULL PRIMARY KEY, VAL1 INTEGER, VAL2 INTEGER) " + tableDDLOptions);
+            conn.createStatement().execute(String.format(
+                "CREATE INDEX %s ON %s (VAL1) INCLUDE (VAL2)", indexTableName, dataTableFullName));
+
+            IndexRegionObserver.setFailDataTableUpdatesForTesting(true);
+
+            PreparedStatement dataPreparedStatement =
+                conn.prepareStatement("UPSERT INTO " + dataTableFullName + " VALUES(?,?,?)");
+            for (int i = 1; i <= NROWS; i++) {
+                dataPreparedStatement.setInt(1, i);
+                dataPreparedStatement.setInt(2, i + 1);
+                dataPreparedStatement.setInt(3, i * 2);
+                dataPreparedStatement.execute();
+            }
+            commitWithException(conn);
+            IndexRegionObserver.setFailDataTableUpdatesForTesting(true);
+
+            IndexTool indexTool = IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.BEFORE, "-fi");
+
+            long actualRowCount = IndexScrutiny.scrutinizeIndex(conn, dataTableFullName, indexTableFullName);
+            assertEquals(0, actualRowCount);
+
+            assertExtraCounters(indexTool, 0, NROWS, true);
+        }
+    }
+
+    @Test
+    public void testPITRow() throws Exception {
+        final int NROWS = 1;
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            initTablesAndAddExtraRowsToIndex(conn, schemaName, dataTableName, indexTableName, NROWS);
+
+            IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.ONLY, "-fi");
+
+            Cell cell = IndexToolIT.getErrorMessageFromIndexToolOutputTable(conn, dataTableFullName, indexTableFullName);
+            try {
+                String expectedErrorMsg = IndexRepairRegionScanner.ERROR_MESSAGE_EXTRA_INDEX_ROW;
+                String actualErrorMsg = Bytes.toString(cell.getValueArray(), cell.getValueOffset(), cell.getValueLength());
+                assertTrue(expectedErrorMsg.equals(actualErrorMsg));
+            } catch(Exception ex) {
+                Assert.fail("Fail to parsing the error message from IndexToolOutputTable");
+            }
+        }
+    }
+
+    @Test
+    public void testVerifyAfterExtraIndexRows() throws Exception {
+        final int NROWS = 20;
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            initTablesAndAddExtraRowsToIndex(conn, schemaName, dataTableName, indexTableName, NROWS);
+
+            // Run -v AFTER and check it doesn't fix the extra rows and the job fails
+            IndexTool indexTool = IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, -1, IndexVerifyType.AFTER, "-fi");

Review comment:
       So this means -fi only works with BEFORE option is that correct?
   Is there any reason why it doesn't work with AFTER? 
   I would expect it to build correctly independent of verify option and Verify is just verification not affecting how we build the index.

##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexRepairRegionScannerIT.java
##########
@@ -163,35 +308,251 @@ public void testRepairExtraIndexRows() throws Exception {
         String indexTableName = generateUniqueName();
         String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
         Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            initTablesAndAddExtraRowsToIndex(conn, schemaName, dataTableName, indexTableName, NROWS);
+
+            // do index rebuild without -fi and check with scrutiny that index tool failed to fix the extra rows
+            IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.BEFORE);
+
+            boolean failed;
+            try {
+                IndexScrutiny.scrutinizeIndex(conn, dataTableFullName, indexTableFullName);
+                failed = false;
+            } catch (AssertionError e) {
+                failed = true;
+            }
+            assertTrue(failed);
+
+            // now repair the index with -fi
+            IndexTool indexTool = IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.BEFORE, "-fi");
+
+            long actualRowCount = IndexScrutiny.scrutinizeIndex(conn, dataTableFullName, indexTableFullName);
+            assertEquals(NROWS, actualRowCount);
+
+            assertExtraCounters(indexTool, NROWS, 0, true);
+        }
+    }
+
+    @Test
+    public void testRepairExtraIndexRows_PostIndexUpdateFailure_overwrite() throws Exception {
+        if (!mutable) {
+            return;
+        }
+        final int NROWS = 4;
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
         try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
             conn.createStatement().execute("CREATE TABLE " + dataTableFullName
-                    + " (ID INTEGER NOT NULL PRIMARY KEY, VAL1 INTEGER, VAL2 INTEGER) "
-                    + tableDDLOptions);
+                + " (ID INTEGER NOT NULL PRIMARY KEY, VAL1 INTEGER, VAL2 INTEGER) " + tableDDLOptions);
+            conn.createStatement().execute(String.format(
+                "CREATE INDEX %s ON %s (VAL1) INCLUDE (VAL2)", indexTableName, dataTableFullName));
+
             PreparedStatement dataPreparedStatement =
-                    conn.prepareStatement("UPSERT INTO " + dataTableFullName + " VALUES(?,?,?)");
+                conn.prepareStatement("UPSERT INTO " + dataTableFullName + " VALUES(?,?,?)");
             for (int i = 1; i <= NROWS; i++) {
                 dataPreparedStatement.setInt(1, i);
                 dataPreparedStatement.setInt(2, i + 1);
                 dataPreparedStatement.setInt(3, i * 2);
                 dataPreparedStatement.execute();
             }
             conn.commit();
+
+            IndexRegionObserver.setFailPostIndexUpdatesForTesting(true);
+            conn.createStatement().execute("UPSERT INTO " + dataTableFullName + " VALUES(3, 100, 200)");
+            conn.commit();
+            IndexRegionObserver.setFailPostIndexUpdatesForTesting(false);
+
+            IndexTool indexTool = IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.BEFORE, "-fi");
+
+            CounterGroup mrJobCounters = IndexToolIT.getMRJobCounters(indexTool);
+            assertEquals(2,
+                mrJobCounters.findCounter(BEFORE_REBUILD_INVALID_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(2,
+                mrJobCounters.findCounter(BEFORE_REBUILD_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_VERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+
+            indexTool = IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.ONLY, "-fi");
+            mrJobCounters = IndexToolIT.getMRJobCounters(indexTool);
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REBUILD_INVALID_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REBUILD_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_VERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+
+            long actualRowCount = IndexScrutiny.scrutinizeIndex(conn, dataTableFullName, indexTableFullName);
+            assertEquals(NROWS, actualRowCount);
+        }
+    }
+
+    @Test
+    public void testRepairExtraIndexRows_PostIndexUpdateFailure_delete() throws Exception {
+        if (!mutable) {
+            return;
+        }
+        final int NROWS = 4;
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            conn.createStatement().execute("CREATE TABLE " + dataTableFullName
+                + " (ID INTEGER NOT NULL PRIMARY KEY, VAL1 INTEGER, VAL2 INTEGER) " + tableDDLOptions);
             conn.createStatement().execute(String.format(
-                    "CREATE INDEX %s ON %s (VAL1) INCLUDE (VAL2)", indexTableName, dataTableFullName));
-            // Add extra index rows
-            PreparedStatement indexPreparedStatement =
-                    conn.prepareStatement("UPSERT INTO " + indexTableFullName + " VALUES(?,?,?)");
+                "CREATE INDEX %s ON %s (VAL1) INCLUDE (VAL2)", indexTableName, dataTableFullName));
 
-            for (int i = NROWS + 1; i <= 2 * NROWS; i++) {
-                indexPreparedStatement.setInt(1, i + 1); // the indexed column
-                indexPreparedStatement.setInt(2, i); // the data pk column
-                indexPreparedStatement.setInt(3, i * 2); // the included column
-                indexPreparedStatement.execute();
+            PreparedStatement dataPreparedStatement =
+                conn.prepareStatement("UPSERT INTO " + dataTableFullName + " VALUES(?,?,?)");
+            for (int i = 1; i <= NROWS; i++) {
+                dataPreparedStatement.setInt(1, i);
+                dataPreparedStatement.setInt(2, i + 1);
+                dataPreparedStatement.setInt(3, i * 2);
+                dataPreparedStatement.execute();
             }
             conn.commit();
-            // Set all index row statuses to verified so that read verify will not fix them. We want them to be fixed
-            // by IndexRepairRegionScanner
-            setIndexRowStatusesToVerified(conn, dataTableFullName, indexTableFullName);
+
+            IndexRegionObserver.setFailPostIndexUpdatesForTesting(true);
+            conn.createStatement().execute("DELETE FROM " + dataTableFullName + " WHERE ID = 3");
+            conn.commit();
+            IndexRegionObserver.setFailPostIndexUpdatesForTesting(false);
+            TestUtil.doMajorCompaction(conn, dataTableFullName);
+
+            IndexTool indexTool = IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.BEFORE, "-fi");
+
+            CounterGroup mrJobCounters = IndexToolIT.getMRJobCounters(indexTool);
+
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REBUILD_INVALID_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REBUILD_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_VERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(1,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+
+            indexTool = IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.ONLY, "-fi");
+            mrJobCounters = IndexToolIT.getMRJobCounters(indexTool);
+
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REBUILD_INVALID_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REBUILD_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_VERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+
+            long actualRowCount = IndexScrutiny.scrutinizeIndex(conn, dataTableFullName, indexTableFullName);
+            assertEquals(NROWS - 1, actualRowCount);
+        }
+    }
+
+    @Test
+    public void testRepairExtraIndexRows_DataTableUpdateFailure() throws Exception {
+        if (!mutable) {
+            return;
+        }
+        final int NROWS = 20;
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            conn.createStatement().execute("CREATE TABLE " + dataTableFullName
+                + " (ID INTEGER NOT NULL PRIMARY KEY, VAL1 INTEGER, VAL2 INTEGER) " + tableDDLOptions);
+            conn.createStatement().execute(String.format(
+                "CREATE INDEX %s ON %s (VAL1) INCLUDE (VAL2)", indexTableName, dataTableFullName));
+
+            IndexRegionObserver.setFailDataTableUpdatesForTesting(true);
+
+            PreparedStatement dataPreparedStatement =
+                conn.prepareStatement("UPSERT INTO " + dataTableFullName + " VALUES(?,?,?)");
+            for (int i = 1; i <= NROWS; i++) {
+                dataPreparedStatement.setInt(1, i);
+                dataPreparedStatement.setInt(2, i + 1);
+                dataPreparedStatement.setInt(3, i * 2);
+                dataPreparedStatement.execute();
+            }
+            commitWithException(conn);
+            IndexRegionObserver.setFailDataTableUpdatesForTesting(true);
+
+            IndexTool indexTool = IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.BEFORE, "-fi");
+
+            long actualRowCount = IndexScrutiny.scrutinizeIndex(conn, dataTableFullName, indexTableFullName);
+            assertEquals(0, actualRowCount);
+
+            assertExtraCounters(indexTool, 0, NROWS, true);
+        }
+    }
+
+    @Test
+    public void testPITRow() throws Exception {
+        final int NROWS = 1;
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            initTablesAndAddExtraRowsToIndex(conn, schemaName, dataTableName, indexTableName, NROWS);
+
+            IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.ONLY, "-fi");
+
+            Cell cell = IndexToolIT.getErrorMessageFromIndexToolOutputTable(conn, dataTableFullName, indexTableFullName);
+            try {
+                String expectedErrorMsg = IndexRepairRegionScanner.ERROR_MESSAGE_EXTRA_INDEX_ROW;
+                String actualErrorMsg = Bytes.toString(cell.getValueArray(), cell.getValueOffset(), cell.getValueLength());
+                assertTrue(expectedErrorMsg.equals(actualErrorMsg));
+            } catch(Exception ex) {
+                Assert.fail("Fail to parsing the error message from IndexToolOutputTable");

Review comment:
       nit: Do you need this? It will fail the test anyway without this assert right? It will be consistent with how we throw exceptions from test methods

##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexRepairRegionScannerIT.java
##########
@@ -163,35 +308,251 @@ public void testRepairExtraIndexRows() throws Exception {
         String indexTableName = generateUniqueName();
         String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
         Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            initTablesAndAddExtraRowsToIndex(conn, schemaName, dataTableName, indexTableName, NROWS);
+
+            // do index rebuild without -fi and check with scrutiny that index tool failed to fix the extra rows
+            IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.BEFORE);
+
+            boolean failed;
+            try {
+                IndexScrutiny.scrutinizeIndex(conn, dataTableFullName, indexTableFullName);
+                failed = false;
+            } catch (AssertionError e) {
+                failed = true;
+            }
+            assertTrue(failed);
+
+            // now repair the index with -fi
+            IndexTool indexTool = IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.BEFORE, "-fi");
+
+            long actualRowCount = IndexScrutiny.scrutinizeIndex(conn, dataTableFullName, indexTableFullName);
+            assertEquals(NROWS, actualRowCount);
+
+            assertExtraCounters(indexTool, NROWS, 0, true);
+        }
+    }
+
+    @Test
+    public void testRepairExtraIndexRows_PostIndexUpdateFailure_overwrite() throws Exception {
+        if (!mutable) {
+            return;
+        }
+        final int NROWS = 4;
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
         try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
             conn.createStatement().execute("CREATE TABLE " + dataTableFullName
-                    + " (ID INTEGER NOT NULL PRIMARY KEY, VAL1 INTEGER, VAL2 INTEGER) "
-                    + tableDDLOptions);
+                + " (ID INTEGER NOT NULL PRIMARY KEY, VAL1 INTEGER, VAL2 INTEGER) " + tableDDLOptions);
+            conn.createStatement().execute(String.format(
+                "CREATE INDEX %s ON %s (VAL1) INCLUDE (VAL2)", indexTableName, dataTableFullName));
+
             PreparedStatement dataPreparedStatement =
-                    conn.prepareStatement("UPSERT INTO " + dataTableFullName + " VALUES(?,?,?)");
+                conn.prepareStatement("UPSERT INTO " + dataTableFullName + " VALUES(?,?,?)");
             for (int i = 1; i <= NROWS; i++) {
                 dataPreparedStatement.setInt(1, i);
                 dataPreparedStatement.setInt(2, i + 1);
                 dataPreparedStatement.setInt(3, i * 2);
                 dataPreparedStatement.execute();
             }
             conn.commit();
+
+            IndexRegionObserver.setFailPostIndexUpdatesForTesting(true);
+            conn.createStatement().execute("UPSERT INTO " + dataTableFullName + " VALUES(3, 100, 200)");
+            conn.commit();
+            IndexRegionObserver.setFailPostIndexUpdatesForTesting(false);
+
+            IndexTool indexTool = IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.BEFORE, "-fi");
+
+            CounterGroup mrJobCounters = IndexToolIT.getMRJobCounters(indexTool);
+            assertEquals(2,
+                mrJobCounters.findCounter(BEFORE_REBUILD_INVALID_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(2,
+                mrJobCounters.findCounter(BEFORE_REBUILD_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_VERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+
+            indexTool = IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.ONLY, "-fi");
+            mrJobCounters = IndexToolIT.getMRJobCounters(indexTool);
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REBUILD_INVALID_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REBUILD_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_VERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+
+            long actualRowCount = IndexScrutiny.scrutinizeIndex(conn, dataTableFullName, indexTableFullName);
+            assertEquals(NROWS, actualRowCount);
+        }
+    }
+
+    @Test
+    public void testRepairExtraIndexRows_PostIndexUpdateFailure_delete() throws Exception {
+        if (!mutable) {
+            return;
+        }
+        final int NROWS = 4;
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            conn.createStatement().execute("CREATE TABLE " + dataTableFullName
+                + " (ID INTEGER NOT NULL PRIMARY KEY, VAL1 INTEGER, VAL2 INTEGER) " + tableDDLOptions);
             conn.createStatement().execute(String.format(
-                    "CREATE INDEX %s ON %s (VAL1) INCLUDE (VAL2)", indexTableName, dataTableFullName));
-            // Add extra index rows
-            PreparedStatement indexPreparedStatement =
-                    conn.prepareStatement("UPSERT INTO " + indexTableFullName + " VALUES(?,?,?)");
+                "CREATE INDEX %s ON %s (VAL1) INCLUDE (VAL2)", indexTableName, dataTableFullName));
 
-            for (int i = NROWS + 1; i <= 2 * NROWS; i++) {
-                indexPreparedStatement.setInt(1, i + 1); // the indexed column
-                indexPreparedStatement.setInt(2, i); // the data pk column
-                indexPreparedStatement.setInt(3, i * 2); // the included column
-                indexPreparedStatement.execute();
+            PreparedStatement dataPreparedStatement =
+                conn.prepareStatement("UPSERT INTO " + dataTableFullName + " VALUES(?,?,?)");
+            for (int i = 1; i <= NROWS; i++) {
+                dataPreparedStatement.setInt(1, i);
+                dataPreparedStatement.setInt(2, i + 1);
+                dataPreparedStatement.setInt(3, i * 2);
+                dataPreparedStatement.execute();
             }
             conn.commit();
-            // Set all index row statuses to verified so that read verify will not fix them. We want them to be fixed
-            // by IndexRepairRegionScanner
-            setIndexRowStatusesToVerified(conn, dataTableFullName, indexTableFullName);
+
+            IndexRegionObserver.setFailPostIndexUpdatesForTesting(true);
+            conn.createStatement().execute("DELETE FROM " + dataTableFullName + " WHERE ID = 3");
+            conn.commit();
+            IndexRegionObserver.setFailPostIndexUpdatesForTesting(false);
+            TestUtil.doMajorCompaction(conn, dataTableFullName);
+
+            IndexTool indexTool = IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.BEFORE, "-fi");
+
+            CounterGroup mrJobCounters = IndexToolIT.getMRJobCounters(indexTool);
+
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REBUILD_INVALID_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REBUILD_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_VERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(1,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+
+            indexTool = IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.ONLY, "-fi");
+            mrJobCounters = IndexToolIT.getMRJobCounters(indexTool);
+
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REBUILD_INVALID_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REBUILD_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_VERIFIED_INDEX_ROW_COUNT.name()).getValue());
+            assertEquals(0,
+                mrJobCounters.findCounter(BEFORE_REPAIR_EXTRA_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
+
+            long actualRowCount = IndexScrutiny.scrutinizeIndex(conn, dataTableFullName, indexTableFullName);
+            assertEquals(NROWS - 1, actualRowCount);
+        }
+    }
+
+    @Test
+    public void testRepairExtraIndexRows_DataTableUpdateFailure() throws Exception {
+        if (!mutable) {
+            return;
+        }
+        final int NROWS = 20;
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            conn.createStatement().execute("CREATE TABLE " + dataTableFullName
+                + " (ID INTEGER NOT NULL PRIMARY KEY, VAL1 INTEGER, VAL2 INTEGER) " + tableDDLOptions);
+            conn.createStatement().execute(String.format(
+                "CREATE INDEX %s ON %s (VAL1) INCLUDE (VAL2)", indexTableName, dataTableFullName));
+
+            IndexRegionObserver.setFailDataTableUpdatesForTesting(true);
+
+            PreparedStatement dataPreparedStatement =
+                conn.prepareStatement("UPSERT INTO " + dataTableFullName + " VALUES(?,?,?)");
+            for (int i = 1; i <= NROWS; i++) {
+                dataPreparedStatement.setInt(1, i);
+                dataPreparedStatement.setInt(2, i + 1);
+                dataPreparedStatement.setInt(3, i * 2);
+                dataPreparedStatement.execute();
+            }
+            commitWithException(conn);
+            IndexRegionObserver.setFailDataTableUpdatesForTesting(true);
+
+            IndexTool indexTool = IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.BEFORE, "-fi");
+
+            long actualRowCount = IndexScrutiny.scrutinizeIndex(conn, dataTableFullName, indexTableFullName);
+            assertEquals(0, actualRowCount);
+
+            assertExtraCounters(indexTool, 0, NROWS, true);
+        }
+    }
+
+    @Test
+    public void testPITRow() throws Exception {
+        final int NROWS = 1;
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            initTablesAndAddExtraRowsToIndex(conn, schemaName, dataTableName, indexTableName, NROWS);
+
+            IndexToolIT.runIndexTool(false, false, schemaName, dataTableName,
+                indexTableName, null, 0, IndexVerifyType.ONLY, "-fi");
+
+            Cell cell = IndexToolIT.getErrorMessageFromIndexToolOutputTable(conn, dataTableFullName, indexTableFullName);
+            try {
+                String expectedErrorMsg = IndexRepairRegionScanner.ERROR_MESSAGE_EXTRA_INDEX_ROW;
+                String actualErrorMsg = Bytes.toString(cell.getValueArray(), cell.getValueOffset(), cell.getValueLength());
+                assertTrue(expectedErrorMsg.equals(actualErrorMsg));

Review comment:
       I recommend having a contains rather than equals here. Sometimes we enhance the error messages.

##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRepairRegionScanner.java
##########
@@ -178,6 +183,28 @@ protected void repairIndexRows(Map<byte[], List<Mutation>> indexMutationMap,
         return actualIndexMutationMap;
     }
 
+    private Map<byte[], List<Mutation>> populateActualIndexMutationMap() throws IOException {
+        Map<byte[], List<Mutation>> actualIndexMutationMap = Maps.newTreeMap(Bytes.BYTES_COMPARATOR);
+        Scan indexScan = new Scan();
+        indexScan.setTimeRange(scan.getTimeRange().getMin(), scan.getTimeRange().getMax());
+        indexScan.setRaw(true);
+        indexScan.setMaxVersions();
+        indexScan.setCacheBlocks(false);
+        try (RegionScanner regionScanner = region.getScanner(indexScan)) {

Review comment:
       Does the below code work if there is no index row? 

##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/GlobalIndexRegionScanner.java
##########
@@ -883,6 +925,9 @@ public boolean verifySingleIndexRow(byte[] indexRowKey, List<Mutation> actualMut
                 logMismatch(expected, actual, expectedIndex, verificationPhaseResult, isBeforeRebuild);
             }
             else {
+                if (expected == null) {

Review comment:
       I think adding a comment here would be nice. Something like this case happens when there is extra index rows




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