Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/277#discussion_r146903705
--- 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 --
> Also, we clear server side cache anytime a SYSTEM table is disabled,
using a coprocessor hook.
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.
> 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.
Is that being down in a CP too? If so, would it make sense to do this
cache-clearing there (instead of client side)?
---
