PHOENIX-7381 : Skip last_ddl_timestamp validation when UCF is defined on table and cache entry is not old

phoenix-core-client/src/main/java/org/apache/phoenix/schema/MetaDataClient.java

@@ -785,13 +785,13 @@ private boolean avoidRpcToGetTable(boolean alwaysHitServer, Long resolvedTimesta
             final long effectiveUpdateCacheFreq;
             final String ucfInfoForLogging; // Only used for logging purposes
 
-            boolean overrideUcfToDefault = false;
+            boolean overrideUcfToAlways = false;
             if (table.getType() == INDEX) {
-                overrideUcfToDefault =
+                overrideUcfToAlways =
                         PIndexState.PENDING_DISABLE.equals(table.getIndexState()) ||
                                 !IndexMaintainer.sendIndexMaintainer(table);
             }
-            if (!overrideUcfToDefault && !table.getIndexes().isEmpty()) {
+            if (!overrideUcfToAlways && !table.getIndexes().isEmpty()) {
                 List<PTable> indexes = table.getIndexes();
                 List<PTable> maintainedIndexes =
                         Lists.newArrayList(IndexMaintainer.maintainedIndexes(indexes.iterator()));
@@ -801,7 +801,7 @@ private boolean avoidRpcToGetTable(boolean alwaysHitServer, Long resolvedTimesta
                 // not in usable state by the client mutations, we should override
                 // UPDATE_CACHE_FREQUENCY to default value so that we make getTable() RPC calls
                 // until all index states change to ACTIVE, BUILDING or other usable states.
-                overrideUcfToDefault = indexes.size() != maintainedIndexes.size();
+                overrideUcfToAlways = indexes.size() != maintainedIndexes.size();
             }
 
             // What if the table is created with UPDATE_CACHE_FREQUENCY explicitly set to ALWAYS?
@@ -810,12 +810,10 @@ private boolean avoidRpcToGetTable(boolean alwaysHitServer, Long resolvedTimesta
 
             //always fetch an Index in PENDING_DISABLE state to retrieve server timestamp
             //QueryOptimizer needs that to decide whether the index can be used
-            if (overrideUcfToDefault) {
+            if (overrideUcfToAlways) {
                 effectiveUpdateCacheFreq =
-                    (Long) ConnectionProperty.UPDATE_CACHE_FREQUENCY.getValue(
-                        connection.getQueryServices().getProps()
-                            .get(QueryServices.DEFAULT_UPDATE_CACHE_FREQUENCY_ATRRIB));
-                ucfInfoForLogging = "connection-level-default";
+                    (Long) ConnectionProperty.UPDATE_CACHE_FREQUENCY.getValue("ALWAYS");
+                ucfInfoForLogging = "override-to-always";
             } else if (table.getUpdateCacheFrequency()
                     != QueryServicesOptions.DEFAULT_UPDATE_CACHE_FREQUENCY) {
                 effectiveUpdateCacheFreq = table.getUpdateCacheFrequency();
@@ -836,9 +834,8 @@ private boolean avoidRpcToGetTable(boolean alwaysHitServer, Long resolvedTimesta
                         (table.getTenantId() != null ? ", Tenant ID: " + table.getTenantId() : ""));
             }
 
-            return (table.getRowTimestampColPos() == -1 &&
-                    connection.getMetaDataCache().getAge(tableRef) <
-                            effectiveUpdateCacheFreq);
+            return MetaDataUtil
+                    .avoidMetadataRPC(connection, table, tableRef, effectiveUpdateCacheFreq);
         }
         return false;
     }

phoenix-core-client/src/main/java/org/apache/phoenix/util/MetaDataUtil.java

@@ -33,6 +33,7 @@
 import org.apache.phoenix.schema.PName;
 import org.apache.phoenix.schema.PNameFactory;
 import org.apache.phoenix.schema.PTable;
+import org.apache.phoenix.schema.PTableRef;
 import org.apache.phoenix.schema.PTableType;
 import org.apache.phoenix.schema.SequenceKey;
 import org.apache.phoenix.schema.SortOrder;
@@ -1196,4 +1197,11 @@ public static long getMaxLookbackAge(Configuration conf, Long maxLookbackAge) {
         return maxLookbackAge != null ? maxLookbackAge :
                 BaseScannerRegionObserverConstants.getMaxLookbackInMillis(conf);
     }
+
+    public static boolean avoidMetadataRPC(PhoenixConnection connection, PTable table,
+                                           PTableRef tableRef, long effectiveUpdateCacheFreq) {
+        return table.getRowTimestampColPos() == -1 &&
+                connection.getMetaDataCache().getAge(tableRef) <
+                        effectiveUpdateCacheFreq;
+    }
 }

phoenix-core-client/src/main/java/org/apache/phoenix/util/ValidateLastDDLTimestampUtil.java

@@ -35,10 +35,13 @@
 import org.apache.phoenix.query.QueryConstants;
 import org.apache.phoenix.query.QueryServices;
 import org.apache.phoenix.query.QueryServicesOptions;
+import org.apache.phoenix.schema.ConnectionProperty;
 import org.apache.phoenix.schema.PName;
 import org.apache.phoenix.schema.PTable;
 import org.apache.phoenix.schema.PTableKey;
+import org.apache.phoenix.schema.PTableRef;
 import org.apache.phoenix.schema.PTableType;
+import org.apache.phoenix.schema.TableNotFoundException;
 import org.apache.phoenix.schema.TableRef;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -100,7 +103,10 @@ public static boolean getValidateLastDdlTimestampEnabled(Configuration config) {
     public static void validateLastDDLTimestamp(PhoenixConnection conn,
                                                 List<TableRef> allTableRefs,
                                                 boolean doRetry) throws SQLException {
-        List<TableRef> tableRefs = filterTableRefs(allTableRefs);
+        List<TableRef> tableRefs = filterTableRefs(conn, allTableRefs);
+        if (tableRefs.isEmpty()) {
+            return;
+        }
         String infoString = getInfoString(conn.getTenantId(), tableRefs);
         try (Admin admin = conn.getQueryServices().getAdmin()) {
             // get all live region servers
@@ -215,14 +221,38 @@ private static void setLastDDLTimestampRequestParameters(
     }
 
     /**
-     * Filter out any TableRefs which are not tables, views or indexes.
+     * Filter out TableRefs for sending to server to validate last_ddl_timestamp.
+     * 1. table type is in ALLOWED_PTABLE_TYPES
+     * 2. table schema has a non-zero UPDATE_CACHE_FREQUENCY and cache entry is old.
      * @param tableRefs
      * @return
      */
-    private static List<TableRef> filterTableRefs(List<TableRef> tableRefs) {
+    private static List<TableRef> filterTableRefs(PhoenixConnection conn,
+                                                  List<TableRef> tableRefs) {
         List<TableRef> filteredTableRefs = tableRefs.stream()
-                .filter(tableRef -> ALLOWED_PTABLE_TYPES.contains(tableRef.getTable().getType()))
+                .filter(tableRef -> ALLOWED_PTABLE_TYPES.contains(tableRef.getTable().getType())
+                            && !avoidRpc(conn, tableRef.getTable()))
                 .collect(Collectors.toList());
         return filteredTableRefs;
     }
+
+    /**
+     * Decide whether we should avoid the validate timestamp RPC for this table. If the schema of
+     * the table had specified a positive UCF to begin with, clients for this table should not see
+     * a regression when metadata caching re-design is enabled i.e. any server RPC should be
+     * skipped for them within the UCF window.
+     */
+    private static boolean avoidRpc(PhoenixConnection conn, PTable table) {
+        try {
+            PTableRef ptr = conn.getTableRef(table.getKey());
+            long tableUCF = table.getUpdateCacheFrequency();
+            return tableUCF > (Long) ConnectionProperty.UPDATE_CACHE_FREQUENCY.getValue("ALWAYS")
+                    && tableUCF < (Long) ConnectionProperty.UPDATE_CACHE_FREQUENCY.getValue("NEVER")
+                    && MetaDataUtil.avoidMetadataRPC(conn, table, ptr, tableUCF);
+        } catch (TableNotFoundException e) {
+            //should not happen since this is called after query compilation and optimizer
+            //so the table would be in the cache
+            return false;
+        }
+    }
 }

phoenix-core/src/it/java/org/apache/phoenix/cache/ServerMetadataCacheIT.java

@@ -1812,6 +1812,47 @@ public void testCacheUpdatedBeforeDDLOperations() throws Exception {
         }
     }
 
+    /**
+     * Test that last_ddl_timestamp validation is skipped when client's operation involves
+     * a table with 0<UCF<Long.MAX_VALUE.
+     */
+    @Test
+    public void testNoDDLTimestampValidationWithTableUCF() throws Exception {
+        String tableName = generateUniqueName();
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+        String url1 = QueryUtil.getConnectionUrl(props, config, "client1");
+        String url2 = QueryUtil.getConnectionUrl(props, config, "client2");
+        ConnectionQueryServices cqs1 = driver.getConnectionQueryServices(url1, props);
+        ConnectionQueryServices cqs2 = driver.getConnectionQueryServices(url2, props);
+        MetricsMetadataCachingSource metricsSource
+                = MetricsPhoenixCoprocessorSourceFactory.getInstance().getMetadataCachingSource();
+        //take a snapshot of current metric values
+        MetricsMetadataCachingSource.MetadataCachingMetricValues oldMetricValues
+                = metricsSource.getCurrentMetricValues();
+
+        try (Connection conn1 = cqs1.connect(url1, props);
+             Connection conn2 = cqs2.connect(url2, props)) {
+
+            // client-1 creates table with UCF=2days
+            createTableWithUCF(conn1, tableName, 2*24*60*60*1000L);
+
+            // query/upsert a few times with same and different client
+            for (int i=0; i<3; i++) {
+                query(conn2, tableName);
+                upsert(conn2, tableName, true);
+                query(conn1, tableName);
+                upsert(conn1, tableName, true);
+            }
+
+            MetricsMetadataCachingSource.MetadataCachingMetricValues newMetricValues
+                    = metricsSource.getCurrentMetricValues();
+
+            assertEquals("There should have been no timestamp validation requests.",
+                    0,
+                    newMetricValues.getValidateDDLTimestampRequestsCount()
+                            - oldMetricValues.getValidateDDLTimestampRequestsCount());
+        }
+    }
 
     //Helper methods
     public static void assertNumGetTableRPC(ConnectionQueryServices spyCqs, String tableName, int numExpectedRPCs) throws SQLException {
@@ -1840,6 +1881,12 @@ private void createTable(Connection conn, String tableName) throws SQLException
                 + "(k INTEGER NOT NULL PRIMARY KEY, v1 INTEGER, v2 INTEGER)");
     }
 
+    private void createTableWithUCF(Connection conn, String tableName, long ucf) throws SQLException {
+        conn.createStatement().execute("CREATE TABLE " + tableName
+                + "(k INTEGER NOT NULL PRIMARY KEY, v1 INTEGER, v2 INTEGER) "
+                + "UPDATE_CACHE_FREQUENCY=" + ucf);
+    }
+
     private void createView(Connection conn, String parentName, String viewName) throws SQLException {
         conn.createStatement().execute("CREATE VIEW " + viewName + " AS SELECT * FROM " + parentName);
     }

phoenix-core/src/it/java/org/apache/phoenix/end2end/UCFWithDisabledIndexWithDDLValidationIT.java

@@ -40,7 +40,7 @@ private static void initCluster() throws Exception {
         Map<String, String> props = Maps.newConcurrentMap();
         props.put(BaseScannerRegionObserverConstants.PHOENIX_MAX_LOOKBACK_AGE_CONF_KEY,
                 Integer.toString(60 * 60 * 1000));
-        props.put(QueryServices.DEFAULT_UPDATE_CACHE_FREQUENCY_ATRRIB, "ALWAYS");
+        props.put(QueryServices.DEFAULT_UPDATE_CACHE_FREQUENCY_ATRRIB, "NEVER");
         props.put(QueryServices.LAST_DDL_TIMESTAMP_VALIDATION_ENABLED, Boolean.toString(true));
         props.put(QueryServices.PHOENIX_METADATA_INVALIDATE_CACHE_ENABLED, Boolean.toString(true));
         props.put(QueryServices.TASK_HANDLING_INITIAL_DELAY_MS_ATTRIB,
@@ -55,26 +55,22 @@ public static synchronized void doSetup() throws Exception {
 
     @Test
     public void testUcfWithNoGetTableCalls() throws Throwable {
-        // Uncomment with PHOENIX-7381
-        //super.testUcfWithNoGetTableCalls();
+        super.testUcfWithNoGetTableCalls();
     }
 
     @Test
     public void testUcfWithDisabledIndex1() throws Throwable {
-        // Uncomment with PHOENIX-7381
-        //super.testUcfWithDisabledIndex1();
+        super.testUcfWithDisabledIndex1();
     }
 
     @Test
     public void testUcfWithDisabledIndex2() throws Throwable {
-        // Uncomment with PHOENIX-7381
-        //super.testUcfWithDisabledIndex2();
+        super.testUcfWithDisabledIndex2();
     }
 
     @Test
     public void testUcfWithDisabledIndex3() throws Throwable {
-        // Uncomment with PHOENIX-7381
-        //super.testUcfWithDisabledIndex3();
+        super.testUcfWithDisabledIndex3();
     }
 
 }