[
https://issues.apache.org/jira/browse/PHOENIX-6129?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17220539#comment-17220539
]
ASF GitHub Bot commented on PHOENIX-6129:
-----------------------------------------
virajjasani commented on a change in pull request #920:
URL: https://github.com/apache/phoenix/pull/920#discussion_r509509727
##########
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:
> This scenario is the same as returning null from the previous
getSysMutexPhysicalTableNameBytes() method and then attempting to retrieve a
null table.
Basically, we are certain that either `SYSTEM.MUTEX` or `SYSTEM:MUTEX` exist
at any point in time right? e.g if we are performing namespace upgrade (e.g
MigrateSystemTablesToSystemNamespaceIT test), when SYSTEM.MUTEX no longer
exists, if we call `HBaseFactoryProvider.getHTableFactory().getTable()`, it
does return `Table` object, it doesn't return null as such. However, using that
table object, we can't perform any operation because actual table would not
exist.
As for this comment, the scenario is not same as previous
`getSysMutexPhysicalTableNameBytes()` because when we reach at this point,
based on above `Admin.tableExists()` call, we would call
`connection.getTable()` with correct table name only (I hope we never have
situation where SYSTEM.MUTEX and SYSTEM:MUTEX both are dropped. Should we
really worry about this case?)
##########
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:
On high level, `connection.getTable()` will never return `null`, it will
return new `Table` object even for non existing tables, so we won't encounter
NPE due to this for sure. However, I believe we should make just one API call
`admin.tableExists()` so that we will know either `SYSTEM.MUTEX` or
`SYSTEM:MUTEX` exist at any time. Sounds good?
----------------------------------------------------------------
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:
us...@infra.apache.org
> Optimize tableExists() call while retrieving correct MUTEX table
> ----------------------------------------------------------------
>
> Key: PHOENIX-6129
> URL: https://issues.apache.org/jira/browse/PHOENIX-6129
> Project: Phoenix
> Issue Type: Improvement
> Affects Versions: 5.0.0, 4.15.0
> Reporter: Chinmay Kulkarni
> Assignee: Viraj Jasani
> Priority: Major
> Labels: phoenix-hardening, quality-improvement
> Fix For: 5.1.0, 4.16.0
>
>
> Inside CQSI.writeMutexCell(), we call
> [getSysMutexPhysicalTableNameBytes()|https://github.com/apache/phoenix/blob/e3c7b4bdce2524eb4fd1e7eb0ccd3454fcca81ce/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java#L4244]
> which in turn calls HBase Admin APIs to check the existence of
> SYSTEM.MUTEX/SYSTEM:MUTEX
> [here|https://github.com/apache/phoenix/blob/e3c7b4bdce2524eb4fd1e7eb0ccd3454fcca81ce/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java#L4309-L4312].
> Later, we anyways try to get the HTable both inside
> [writeMutexCell()|https://github.com/apache/phoenix/blob/e3c7b4bdce2524eb4fd1e7eb0ccd3454fcca81ce/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java#L4245]
> and
> [deleteMutexCell()|https://github.com/apache/phoenix/blob/e3c7b4bdce2524eb4fd1e7eb0ccd3454fcca81ce/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java#L4289].
> Instead, we can lazily fail this getTable() call (by catching the HBase
> TableNotFoundException) to avoid additional HBase Admin API calls.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
