[ 
https://issues.apache.org/jira/browse/PHOENIX-6200?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17242594#comment-17242594
 ] 

ASF GitHub Bot commented on PHOENIX-6200:
-----------------------------------------

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:
[email protected]


> Add counters for extra index rows, log results to PIT and PIT_RESULT table
> --------------------------------------------------------------------------
>
>                 Key: PHOENIX-6200
>                 URL: https://issues.apache.org/jira/browse/PHOENIX-6200
>             Project: Phoenix
>          Issue Type: Sub-task
>            Reporter: Tanuj Khurana
>            Assignee: Tanuj Khurana
>            Priority: Major
>




--
This message was sent by Atlassian Jira
(v8.3.4#803005)

Reply via email to