Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/277#discussion_r146924391
--- 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 --
> My assumption was that the intent was to drop any cached PTables if we
moved any system tables around. Given the above, it sounds like this is (now?)
unnecessary.
That's correct. We indeed need to remove it from cache because once the
migration is complete, we add a row in `SYSCAT` indicating that
`IS_NAMESPACE_MAPPED` to true. Although it seems unnecessary, I don't really
want to rely on the coprocessor hook as it may change any time. An explicit
`clearCache()` should not harm it, I believe.
> Is that being down in a CP too? If so, would it make sense to do this
cache-clearing there (instead of client side)?
What is CP? I didn't quite get you.
---
