[
https://issues.apache.org/jira/browse/PHOENIX-6129?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17218452#comment-17218452
]
ASF GitHub Bot commented on PHOENIX-6129:
-----------------------------------------
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:
us...@infra.apache.org
> Remove unnecessary HBase admin API calls when writing and deleting mutex cells
> ------------------------------------------------------------------------------
>
> 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)
