[GitHub] [phoenix] gjacoby126 commented on a change in pull request #789: PHOENIX-5783: Implement starttime in IndexTool for rebuild and verifi…

[GitBox <gi...@apache.org>]

gjacoby126 commented on a change in pull request #789:
URL: https://github.com/apache/phoenix/pull/789#discussion_r432646729



##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolForNonTxGlobalIndexIT.java
##########
@@ -588,9 +588,101 @@ public void testIndexToolForIncrementalRebuild() throws Exception {
         }
     }
 
+    @Test
+    public void testIndexToolForIncrementalVerify() throws Exception {
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String indexTableName = generateUniqueName();
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            conn.setAutoCommit(true);
+            conn.createStatement().execute("CREATE TABLE "+dataTableFullName+" "
+                    + "(key1 BIGINT NOT NULL, key2 BIGINT NOT NULL, val1 VARCHAR, val2 BIGINT, "
+                    + "val3 BIGINT, val4 DOUBLE, val5 BIGINT, val6 VARCHAR "
+                    + "CONSTRAINT my_pk PRIMARY KEY(key1, key2)) "+tableDDLOptions);
+            conn.createStatement().execute(String.format(
+                    "CREATE INDEX "+indexTableName+" ON "+dataTableFullName+" (val3) INCLUDE(val5)"));
+            long t0 = EnvironmentEdgeManager.currentTimeMillis();

Review comment:
       Is the edge manager injected somewhere? Otherwise this seems prone to flapping if the wall clock milliseconds don't act quite the way you're expecting. Suggest either injecting a manual edge, or picking fixed timestamps for each time variable. 

##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java
##########
@@ -1081,11 +1086,39 @@ private RegionScanner rebuildIndices(final RegionScanner innerScanner, final Reg
             rawScan.setRaw(true);
             rawScan.setMaxVersions();
             rawScan.getFamilyMap().clear();
-            rawScan.setFilter(null);
             rawScan.setCacheBlocks(false);
             for (byte[] family : scan.getFamilyMap().keySet()) {
                 rawScan.addFamily(family);
             }
+            rawScan.setFilter(null);
+            //override the filter to skip scan when lower bound of timerange is passed

Review comment:
       More comments please about why we're scanning through everything twice. I figured it out (grabbing the keys that existed at minTime, then doing a skip scan of just those keys), but I had to stare at it for a few min. :-) 




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