palashc commented on code in PR #1702:
URL: https://github.com/apache/phoenix/pull/1702#discussion_r1355844117
##########
phoenix-core/src/test/java/org/apache/phoenix/cache/ServerMetadataCacheTest.java:
##########
@@ -420,6 +423,126 @@ public void
testInvalidateCacheForBaseTableWithUpdateIndexStatement() throws Exc
}
+ /**
+ * Test that we invalidate the cache for parent table and update the last
ddl timestamp
+ * of the parent table while we add an index.
+ * Test that we invalidate the cache for parent table and index when we
drop an index.
+ * Also we update the last ddl timestamp for parent table when we drop an
index.
+ * @throws Exception
+ */
+ @Test
+ public void testUpdateLastDDLTimestampTableAfterIndexCreation() throws
Exception {
+ String tableName = generateUniqueName();
+ byte[] tableNameBytes = Bytes.toBytes(tableName);
+ String indexName = generateUniqueName();
+ byte[] indexNameBytes = Bytes.toBytes(indexName);
+ ServerMetadataCache cache = ServerMetadataCache.getInstance(config);
+ String ddl =
+ "create table " + tableName + " ( k integer PRIMARY KEY," + "
v1 integer,"
+ + " v2 integer)";
+ String createIndexDDL = "create index " + indexName + " on " +
tableName + " (v1)";
+ String dropIndexDDL = "DROP INDEX " + indexName + " ON " + tableName;
+ try (Connection conn = DriverManager.getConnection(getUrl());
+ Statement stmt = conn.createStatement()) {
+ conn.setAutoCommit(true);
+ stmt.execute(ddl);
+ long tableLastDDLTimestampBeforeIndexCreation =
getLastDDLTimestamp(tableName);
+ // Populate the cache
+ assertNotNull(cache.getLastDDLTimestampForTable(null, null,
tableNameBytes));
+ Thread.sleep(1);
Review Comment:
For my understanding, why is sleep required in these tests?
##########
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java:
##########
@@ -3507,23 +3525,30 @@ private void
invalidateServerMetadataCacheWithRetries(Admin admin,
// DDL operations. We also need to think of we need
separate RPC handler
// threads for this?
ServerRpcController controller = new ServerRpcController();
+ for (InvalidateServerMetadataCacheRequest
invalidateCacheRequest:
Review Comment:
We are already iterating over the list of
`InvalidateServerMetadataCacheRequests` in `getRequest` to create the proto
request. Can we do the logging there? The list is very small so maybe it does
not matter?
##########
phoenix-core/src/test/java/org/apache/phoenix/cache/ServerMetadataCacheTest.java:
##########
@@ -420,6 +423,126 @@ public void
testInvalidateCacheForBaseTableWithUpdateIndexStatement() throws Exc
}
+ /**
+ * Test that we invalidate the cache for parent table and update the last
ddl timestamp
+ * of the parent table while we add an index.
+ * Test that we invalidate the cache for parent table and index when we
drop an index.
+ * Also we update the last ddl timestamp for parent table when we drop an
index.
+ * @throws Exception
+ */
+ @Test
+ public void testUpdateLastDDLTimestampTableAfterIndexCreation() throws
Exception {
+ String tableName = generateUniqueName();
+ byte[] tableNameBytes = Bytes.toBytes(tableName);
+ String indexName = generateUniqueName();
+ byte[] indexNameBytes = Bytes.toBytes(indexName);
+ ServerMetadataCache cache = ServerMetadataCache.getInstance(config);
+ String ddl =
+ "create table " + tableName + " ( k integer PRIMARY KEY," + "
v1 integer,"
+ + " v2 integer)";
+ String createIndexDDL = "create index " + indexName + " on " +
tableName + " (v1)";
+ String dropIndexDDL = "DROP INDEX " + indexName + " ON " + tableName;
+ try (Connection conn = DriverManager.getConnection(getUrl());
+ Statement stmt = conn.createStatement()) {
+ conn.setAutoCommit(true);
+ stmt.execute(ddl);
+ long tableLastDDLTimestampBeforeIndexCreation =
getLastDDLTimestamp(tableName);
+ // Populate the cache
+ assertNotNull(cache.getLastDDLTimestampForTable(null, null,
tableNameBytes));
+ Thread.sleep(1);
+ stmt.execute(createIndexDDL);
+ // Make sure that we have invalidated the last ddl timestamp for
parent table
+ // on all regionservers after we create an index.
+ assertNull(cache.getLastDDLTimestampForTableFromCacheOnly(null,
null, tableNameBytes));
+ long tableLastDDLTimestampAfterIndexCreation =
getLastDDLTimestamp(tableName);
+ assertNotNull(tableLastDDLTimestampAfterIndexCreation);
+ assertTrue(tableLastDDLTimestampAfterIndexCreation >
+ tableLastDDLTimestampBeforeIndexCreation);
+ long indexLastDDLTimestampAfterCreation =
getLastDDLTimestamp(indexName);
+ // Make sure that last ddl timestamp is cached on the regionserver.
+ assertNotNull(indexLastDDLTimestampAfterCreation);
+ Thread.sleep(1);
+ stmt.execute(dropIndexDDL);
+ // Make sure that we invalidate the cache on regionserver for base
table and an index
+ // after we dropped an index.
+ assertNull(cache.getLastDDLTimestampForTableFromCacheOnly(null,
null, tableNameBytes));
+ assertNull(cache.getLastDDLTimestampForTableFromCacheOnly(null,
null, indexNameBytes));
+ long tableLastDDLTimestampAfterIndexDeletion =
getLastDDLTimestamp(tableName);
+ // Verify that last ddl timestamp after index deletion is greater
than
+ // the previous last ddl timestamp.
+ assertNotNull(tableLastDDLTimestampAfterIndexDeletion);
+ assertTrue(tableLastDDLTimestampAfterIndexDeletion >
+ tableLastDDLTimestampAfterIndexCreation);
+ }
+ }
+
+ /**
+ * Test that we invalidate the cache of the immediate parent view
Review Comment:
We are also testing the drop-index case so the comment can be updated.
##########
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/InvalidateServerMetadataCacheRequest.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.coprocessor;
+
+public class InvalidateServerMetadataCacheRequest {
Review Comment:
Maybe a `toString` method could be useful when we need to log the contents
of this request?
##########
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/InvalidateServerMetadataCacheRequest.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.coprocessor;
+
+public class InvalidateServerMetadataCacheRequest {
+ private final byte[] tenantId;
+ private final byte[] schema;
+ private final byte[] name;
+
+ public InvalidateServerMetadataCacheRequest(byte[] tenantId, byte[]
schema, byte[] name) {
+ this.tenantId = tenantId;
+ this.schema = schema;
+ this.name = name;
Review Comment:
nit: Should these be `schemaName` and `tableName` ?
--
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]
