Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-6129 : Optimize tableExists() call while retrieving correct MUTEX table - #920
Conversation
virajjasani
commented
Oct 14, 2020
Since it is not allowed to mutate SYSTEM tables, providing corresponding IT test seems difficult. |
…cquireUpgradeMutex()
…) from acquireUpgradeMutex()" This reverts commit 9aafabf.
| SchemaUtil.getTableKey(tenantId, schemaName, tableName); | ||
| // at this point the system mutex table should have been created or | ||
| // an exception thrown | ||
| byte[] sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes(); |
There was a problem hiding this comment.
byte[] sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes();
If I understand correctly what @ChinmaySKulkarni described in the ticket, this call will still result to an admin.tableExists call to check the existance of SYSTEM.MUTEX/SYSTEM:MUTEX and you didn't changed that.
There was a problem hiding this comment.
try (Table sysMutexTable = getTable(sysMutexPhysicalTableNameBytes)) {
Instead We could try the Table sysMutexTable =getTable() call with one of them and catch HBase TableNotFoundException, in that case try with the other one.
There was a problem hiding this comment.
If getSysMutexPhysicalTableNameBytes() start throwing TableNotFoundException, then at this point the Exception would be thrown and caught, so we would not go ahead with next getTable() call.
There was a problem hiding this comment.
There was a problem hiding this comment.
@virajjasani I think there is some confusion here. The aim of this Jira is to reduce the HBase calls to get the table. Currently, the call to writeMutexCell() calls getSysMutexPhysicalTableNameBytes() which does 1 or 2 HBase admin calls (tableExists()) and then we still do a getTable() call here.
The same happens for deleteMutexCell().
Instead of calling getSysMutexPhysicalTableNameBytes(), we can do 1 getTable() call with SYSTEM.MUTEX and if that throws a TNFE, try again with SYSTEM:MUTEX thus eliminating the tableExists() calls.
There was a problem hiding this comment.
My bad for this misunderstanding. Addressed concerns, updated the PR.
stoty
commented
Oct 15, 2020
💔 -1 overall
This message was automatically generated. |
Test failures don't seem relevant, tried locally, all passed. Fixed checkstyle issues relevant to this PR. |
stoty
commented
Oct 15, 2020
💔 -1 overall
This message was automatically generated. |
stoty
commented
Oct 16, 2020
💔 -1 overall
This message was automatically generated. |
virajjasani
commented
Oct 16, 2020
@ChinmaySKulkarni IMHO we can not live without Admin.tableExists is the only thread safe way to identify if table already exists: & I checked implementation also, and getTable() internally uses We can do one improvement though, we can utilize same connection to perform both: tableExists() and getTable(). |
stoty
commented
Oct 16, 2020
💔 -1 overall
This message was automatically generated. |
stoty
commented
Oct 16, 2020
💔 -1 overall
This message was automatically generated. |
stoty
commented
Oct 20, 2020
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Added some comments. One generic question regarding your finding:
Connection.getTable() has no guarantee that it will throw TableNotFoundException if table doesn't exist.
CQSI.getTable() depends on HBaseFactoryProvider.getHTableFactory().getTable() which internally also uses connection.getTable(). As you can see from the CQSI.getTable() code, it tries to catch TableNotFoundException and probably expects one to be thrown if the table doesn't exist. As per your findings however, this is not guaranteed. This is a problem right? Do we need a separate Jira for this (if yes, can you please open one and mention your findings there)
Uh oh!
There was an error while loading. Please reload this page.
| } 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 { |
There was a problem hiding this comment.
Please add a unit test for this new method.
Uh oh!
There was an error while loading. Please reload this page.
ChinmaySKulkarni
commented
Oct 21, 2020
@virajjasani please also make the commit message the same as the Jira (or update the Jira if you think this is more appropriate). |
virajjasani
commented
Oct 21, 2020
@ChinmaySKulkarni I have addressed your comments, updated Jira message and created Jira PHOENIX-6203 for TNFE issue. Thanks for the review! |
stoty
commented
Oct 21, 2020
💔 -1 overall
This message was automatically generated. |
stoty
commented
Oct 22, 2020
💔 -1 overall
This message was automatically generated. |
virajjasani
commented
Oct 23, 2020
@ChinmaySKulkarni please take a look when you get time. Failed tests are not relevant. |
ChinmaySKulkarni
left a comment
There was a problem hiding this comment.
+1. Thanks for fixing this @virajjasani !
No description provided.