shahrs87 commented on code in PR #1859:
URL: https://github.com/apache/phoenix/pull/1859#discussion_r1562878900
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/AlterTableWithViewsIT.java:
##########
@@ -1149,10 +1152,8 @@ public void testDroppingIndexedColDropsViewIndex()
throws Exception {
byte[] viewIndexPhysicalTable =
viewIndex.getPhysicalName().getBytes();
assertNotNull("Can't find view index", viewIndex);
assertEquals("Unexpected number of indexes ", 2,
view.getIndexes().size());
- assertEquals("Unexpected index ", fullNameViewIndex1 ,
view.getIndexes().get(0).getName()
Review Comment:
Why is this change needed?
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/AlterTableWithViewsIT.java:
##########
@@ -1174,21 +1175,16 @@ public void testDroppingIndexedColDropsViewIndex()
throws Exception {
}
// verify index metadata was dropped
- try {
- viewConn.createStatement().execute("SELECT * FROM " +
fullNameViewIndex1 );
- fail("Index metadata should have been dropped");
- } catch (TableNotFoundException e) {
- }
-
pconn = viewConn.unwrap(PhoenixConnection.class);
- view = pconn.getTable(new PTableKey(tenantId, viewOfTable ));
+ view = pconn.getTableNoCache(viewOfTable);
+ assertEquals("Unexpected number of indexes ", 1,
view.getIndexes().size());
+ assertEquals("Unexpected index ", fullNameViewIndex2 ,
view.getIndexes().get(0).getName().getString());
+ assertNotEquals("Dropped index should not be in view metadata ",
fullNameViewIndex1 , view.getIndexes().get(0).getName().getString());
try {
- viewIndex = pconn.getTable(new PTableKey(tenantId,
fullNameViewIndex1 ));
+ viewIndex = pconn.getTableNoCache(fullNameViewIndex1);
Review Comment:
why is this change required?
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/LogicalTableNameBaseIT.java:
##########
@@ -530,13 +531,16 @@ protected void validateIndex(Connection connection,
String tableName, boolean is
}
public static void renameAndDropPhysicalTable(Connection conn, String
tenantId, String schema, String tableName, String physicalName, boolean
isNamespaceEnabled) throws Exception {
+ // if client is validating last_ddl_timestamp, this change in physical
table name should be visible to the client
Review Comment:
Do we need this change given that default UCF is set to ALWAYS?
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/AlterTableWithViewsIT.java:
##########
@@ -1174,21 +1175,16 @@ public void testDroppingIndexedColDropsViewIndex()
throws Exception {
}
// verify index metadata was dropped
- try {
Review Comment:
why is this change required?
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/ViewMetadataIT.java:
##########
@@ -1173,10 +1174,10 @@ private void
helpTestQueryForViewOnTableThatHasIndex(Statement s1,
s1.execute("create index " + indexName + " ON " + tableName
+ " (col2)");
- try (ResultSet rs = s2.executeQuery("explain select /*+ INDEX("
+ try (ResultSet rs = s2.executeQuery("select /*+ INDEX("
Review Comment:
why this change?
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexAsyncThresholdIT.java:
##########
@@ -162,9 +162,7 @@ public void testAsyncIndexCreation() throws Exception {
exception = (SQLException) e;
}
connection.commit();
- PTableKey key = new PTableKey(null, this.tableName);
- PMetaData metaCache =
connection.unwrap(PhoenixConnection.class).getMetaDataCache();
- List<PTable> indexes =
metaCache.getTableRef(key).getTable().getIndexes();
+ List<PTable> indexes =
connection.unwrap(PhoenixConnection.class).getTable(this.tableName).getIndexes();
Review Comment:
why this change?
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/LogicalTableNameExtendedIT.java:
##########
@@ -209,21 +209,26 @@ public void testHint() throws Exception {
createIndexOnTable(conn, tableName, indexName2);
populateTable(conn, tableName, 1, 2);
- // Test hint
String tableSelect = "SELECT V1,V2,V3 FROM " + tableName;
ResultSet rs = conn.createStatement().executeQuery("EXPLAIN " +
tableSelect);
- assertEquals(true,
QueryUtil.getExplainPlan(rs).contains(indexName));
+ String plan = QueryUtil.getExplainPlan(rs);
Review Comment:
why is this change required?
##########
phoenix-core/src/test/java/org/apache/phoenix/jdbc/ParallelPhoenixConnectionFailureTest.java:
##########
@@ -51,9 +52,12 @@ public class ParallelPhoenixConnectionFailureTest extends
BaseTest {
@Test
public void testExecuteQueryChainFailure() throws SQLException {
HBaseTestingUtility hbaseTestingUtility = new HBaseTestingUtility();
-
- PhoenixConnection conn1 = (PhoenixConnection)
DriverManager.getConnection(url);
- PhoenixConnection conn2 = (PhoenixConnection)
DriverManager.getConnection(url);
+ Properties props = new Properties();
+ // disable last_ddl_timestamp validation for tests which use
ConnectionLessQueryService
+ // since there is no hbase connection to get list of live region
servers
+ props.put(QueryServices.LAST_DDL_TIMESTAMP_VALIDATION_ENABLED,
Boolean.toString(false));
Review Comment:
The default is false. Do we need to set it to false again?
##########
phoenix-core/src/it/java/org/apache/phoenix/query/MetaDataCachingIT.java:
##########
@@ -142,18 +146,25 @@ public void testSystemTablesAreInCache() throws Exception
{
*/
@Test
public void testGlobalClientCacheMetrics() throws Exception {
- int numThreads = 5;
+ int numThreads = 1;
Review Comment:
why this change?
##########
phoenix-core/src/test/java/org/apache/phoenix/query/BaseConnectionlessQueryTest.java:
##########
@@ -86,7 +88,11 @@ private static void startServer(String url) throws Exception
{
// only load the test driver if we are testing locally - for
integration tests, we want to
// test on a wider scale
if (PhoenixEmbeddedDriver.isTestUrl(url)) {
- driver = initDriver(ReadOnlyProps.EMPTY_PROPS);
+ Map<String, String> props = Maps.newHashMapWithExpectedSize(1);
+ // disable last_ddl_timestamp validation for tests which use
ConnectionLessQueryService
+ // since there is no hbase connection to get list of live region
servers
+ props.put(QueryServices.LAST_DDL_TIMESTAMP_VALIDATION_ENABLED,
Boolean.toString(false));
Review Comment:
The default is false. Do we need to set it to false again?
##########
phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixTableLevelMetricsIT.java:
##########
@@ -1276,14 +1281,24 @@ public void testMetricsWithIndexUsage() throws
Exception {
assertTrue(metricExists);
metricExists = false;
//assert BaseTable is not being queried
- for (PhoenixTableMetric metric :
getPhoenixTableClientMetrics().get(dataTable)) {
- if (metric.getMetricType().equals(SELECT_SQL_COUNTER)) {
- metricExists = true;
- assertMetricValue(metric, SELECT_SQL_COUNTER, 0,
CompareOp.EQ);
- break;
+ //if client is validating last_ddl_timestamps with ucf=never,
+ //there will be no metrics for base table (like getTable RPC
times/counts).
Review Comment:
Can you explain more on this?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
