Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 5 commits into from
Oct 1, 2024
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4f8b056
skip ddl validation when UCF is defined on table and cache entry is n…
Aug 13, 2024
e899baa
case when table is not found in cache
Aug 13, 2024
3ebe298
Merge branch 'master' into PHOENIX-7381
Sep 30, 2024
8bc878d
small refactor and fix tests
Sep 30, 2024
2af5bee
change log info when overriding ucf
Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 9 additions & 12 deletions phoenix-core-client/src/main/java/org/apache/phoenix/schema/MetaDataClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Expand All @@ -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?
Expand All @@ -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 = "index-level-override";
palashc marked this conversation as resolved.
Show resolved Hide resolved
} else if (table.getUpdateCacheFrequency()
!= QueryServicesOptions.DEFAULT_UPDATE_CACHE_FREQUENCY) {
effectiveUpdateCacheFreq = table.getUpdateCacheFrequency();
Expand All @@ -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;
}
Expand Down
8 changes: 8 additions & 0 deletions phoenix-core-client/src/main/java/org/apache/phoenix/util/MetaDataUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
}
38 changes: 34 additions & 4 deletions phoenix-core-client/src/main/java/org/apache/phoenix/util/ValidateLastDDLTimestampUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
}
}
47 changes: 47 additions & 0 deletions phoenix-core/src/it/java/org/apache/phoenix/cache/ServerMetadataCacheIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Comment on lines +1850 to +1853
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newMetricValues.getValidateDDLTimestampRequestsCount() should also be 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will be requests from other tests, that's why I am checking the difference before and after just this test.

}
}

//Helper methods
public static void assertNumGetTableRPC(ConnectionQueryServices spyCqs, String tableName, int numExpectedRPCs) throws SQLException {
Expand Down Expand Up @@ -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);
}
Expand Down
14 changes: 5 additions & 9 deletions ...-core/src/it/java/org/apache/phoenix/end2end/UCFWithDisabledIndexWithDDLValidationIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to test with both ALWAYS and NEVER?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@virajjasani With DDL timestamp validation, UCF should not be ALWAYS, so I think it is okay. WDYT?

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,
Expand All @@ -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();
}

}