You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by GitBox <gi...@apache.org> on 2020/10/21 17:42:08 UTC
[GitHub] [phoenix] ChinmaySKulkarni commented on a change in pull request #920: PHOENIX-6129 : Optimize tableExists() call while retrieving correct MUTEX table
ChinmaySKulkarni commented on a change in pull request #920:
URL: https://github.com/apache/phoenix/pull/920#discussion_r509482018
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4364,17 +4353,17 @@ public void deleteMutexCell(String tenantId, String schemaName, String tableName
}
}
- private byte[] getSysMutexPhysicalTableNameBytes() throws IOException, SQLException {
- byte[] sysMutexPhysicalTableNameBytes = null;
- try(Admin admin = getAdmin()) {
- if(admin.tableExists(PhoenixDatabaseMetaData.SYSTEM_MUTEX_HBASE_TABLE_NAME)) {
- sysMutexPhysicalTableNameBytes = PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME_BYTES;
- } else if (admin.tableExists(TableName.valueOf(
- SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName()))) {
- sysMutexPhysicalTableNameBytes = SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName();
+ private Table getSysMutexTable() throws SQLException, IOException {
+ String table = SYSTEM_MUTEX_NAME;
+ TableName tableName = TableName.valueOf(table);
+ try (Admin admin = getAdmin()) {
+ if (!admin.tableExists(tableName)) {
+ table = table.replace(QueryConstants.NAME_SEPARATOR,
+ QueryConstants.NAMESPACE_SEPARATOR);
+ tableName = TableName.valueOf(table);
}
+ return connection.getTable(tableName);
Review comment:
Just want to clarify one thing based on your comment about `connection.getTable()`. At this point, SYS.MUTEX doesn't exist so we try to retrieve SYS:MUTEX. As per your findings, connection.getTable(SYS:MUTEX) would return a `Table` object however it is not guaranteed that the table actually exists. This scenario is the same as returning null from the previous `getSysMutexPhysicalTableNameBytes()` method and then attempting to retrieve a `null` table.
Can you confirm that this scenario (probably an impossible one albeit) is safe and we don't run into any weirdness?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4364,17 +4353,17 @@ public void deleteMutexCell(String tenantId, String schemaName, String tableName
}
}
- private byte[] getSysMutexPhysicalTableNameBytes() throws IOException, SQLException {
- byte[] sysMutexPhysicalTableNameBytes = null;
- try(Admin admin = getAdmin()) {
- if(admin.tableExists(PhoenixDatabaseMetaData.SYSTEM_MUTEX_HBASE_TABLE_NAME)) {
- sysMutexPhysicalTableNameBytes = PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME_BYTES;
- } else if (admin.tableExists(TableName.valueOf(
- SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName()))) {
- sysMutexPhysicalTableNameBytes = SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName();
+ private Table getSysMutexTable() throws SQLException, IOException {
+ String table = SYSTEM_MUTEX_NAME;
Review comment:
nit: `table` variable is unnecessary here
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4364,17 +4353,17 @@ public void deleteMutexCell(String tenantId, String schemaName, String tableName
}
}
- private byte[] getSysMutexPhysicalTableNameBytes() throws IOException, SQLException {
- byte[] sysMutexPhysicalTableNameBytes = null;
- try(Admin admin = getAdmin()) {
- if(admin.tableExists(PhoenixDatabaseMetaData.SYSTEM_MUTEX_HBASE_TABLE_NAME)) {
- sysMutexPhysicalTableNameBytes = PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME_BYTES;
- } else if (admin.tableExists(TableName.valueOf(
- SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName()))) {
- sysMutexPhysicalTableNameBytes = SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName();
+ private Table getSysMutexTable() throws SQLException, IOException {
Review comment:
Please add a unit test for this new method.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org