Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/277#discussion_r146697994
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
---
@@ -3101,49 +3124,68 @@ void ensureSystemTablesUpgraded(ReadOnlyProps props)
// Regardless of the case 1 or 2, if the NS does not
exist, we will error expectedly
// below. If the NS does exist and is mapped, the below
check will exit gracefully.
}
-
+
List<TableName> tableNames = getSystemTableNames(admin);
// No tables exist matching "SYSTEM\..*", they are all already
in "SYSTEM:.*"
if (tableNames.size() == 0) { return; }
// Try to move any remaining tables matching "SYSTEM\..*" into
"SYSTEM:"
if (tableNames.size() > 5) {
logger.warn("Expected 5 system tables but found " +
tableNames.size() + ":" + tableNames);
}
+
+ // Try acquiring a lock in SYSMUTEX table before migrating the
tables since it involves disabling the table
+ // If we cannot acquire lock, it means some old client is
either migrating SYSCAT or trying to upgrade the
+ // schema of SYSCAT table and hence it should not be
interrupted
+ acquiredMutexLock = acquireUpgradeMutex(0, mutexRowKey);
+ if(acquiredMutexLock) {
+ logger.debug("Acquired lock in SYSMUTEX table for
migrating SYSTEM tables to SYSTEM namespace");
+ }
+ // We will not reach here if we fail to acquire the lock,
since it throws UpgradeInProgressException
+
+ // Handle the upgrade of SYSMUTEX table separately since it
doesn't have any entries in SYSCAT
+ String sysMutexSrcTableName =
PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME;
+ String sysMutexDestTableName =
SchemaUtil.getPhysicalName(sysMutexSrcTableName.getBytes(),
props).getNameAsString();
+ UpgradeUtil.mapTableToNamespace(admin, sysMutexSrcTableName,
sysMutexDestTableName, PTableType.SYSTEM);
+
byte[] mappedSystemTable = SchemaUtil
.getPhysicalName(PhoenixDatabaseMetaData.SYSTEM_CATALOG_NAME_BYTES,
props).getName();
metatable = getTable(mappedSystemTable);
if
(tableNames.contains(PhoenixDatabaseMetaData.SYSTEM_CATALOG_HBASE_TABLE_NAME)) {
if (!admin.tableExists(mappedSystemTable)) {
+ // Actual migration of SYSCAT table
UpgradeUtil.mapTableToNamespace(admin, metatable,
PhoenixDatabaseMetaData.SYSTEM_CATALOG_NAME,
props, null, PTableType.SYSTEM,
null);
+ // Invalidate the client-side metadataCache
ConnectionQueryServicesImpl.this.removeTable(null,
PhoenixDatabaseMetaData.SYSTEM_CATALOG_NAME,
null,
MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP_4_1_0);
}
tableNames.remove(PhoenixDatabaseMetaData.SYSTEM_CATALOG_HBASE_TABLE_NAME);
}
-
tableNames.remove(PhoenixDatabaseMetaData.SYSTEM_MUTEX_HBASE_TABLE_NAME);
for (TableName table : tableNames) {
UpgradeUtil.mapTableToNamespace(admin, metatable,
table.getNameAsString(), props, null, PTableType.SYSTEM,
null);
ConnectionQueryServicesImpl.this.removeTable(null,
table.getNameAsString(), null,
MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP_4_1_0);
}
- if (!tableNames.isEmpty()) {
- clearCache();
- }
+
+ // Clear the server-side metadataCache when all tables are
migrated so that the new PTable can be loaded with NS mapping
+ clearCache();
--- End diff --
if (!tableNames.isEmpty()) {
clearCache();
}
I don't really understand why it was done this way, but since we are not
removing entries from the `tableNames` variable, this piece of code will always
get executed and clear the server side cache.
Also, we clear server side cache anytime a SYSTEM table is disabled, using
a coprocessor hook.
So I decided to simplify the logic and clear the cache every time. Its good
to do that because the server side cache will go out of sync with the SYSCAT
table once the migration happens.
For example, if the cached Ptable represented SYSTEM.CATALOG
(IS_NAMESPACE_MAPPED=false), the new Ptable should represent SYSTEM:CATALOG
(IS_NAMESPACE_MAPPED=true). This can only happen if the cache entry for that
table is invalidated.
---
