From 5e3b5d9720a39519b1710642ea2dd19b2ad86b75 Mon Sep 17 00:00:00 2001 From: Viraj Jasani Date: Wed, 14 Oct 2020 17:10:37 +0530 Subject: [PATCH 1/6] PHOENIX-6129 : Avoid redundant Admin API call in the absence of SYSTEM.MUTEX table --- .../query/ConnectionQueryServicesImpl.java | 36 +++++++++---------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java b/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java index 526382b3afa..a91087d28c1 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java @@ -4277,10 +4277,11 @@ public boolean acquireUpgradeMutex(long currentServerSideTableTimestamp) throws IOException, SQLException { Preconditions.checkArgument(currentServerSideTableTimestamp < MIN_SYSTEM_TABLE_TIMESTAMP); - byte[] sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes(); - if(sysMutexPhysicalTableNameBytes == null) { + try { + getSysMutexPhysicalTableNameBytes(); + } catch (TableNotFoundException e) { throw new UpgradeInProgressException(getVersion(currentServerSideTableTimestamp), - getVersion(MIN_SYSTEM_TABLE_TIMESTAMP)); + getVersion(MIN_SYSTEM_TABLE_TIMESTAMP)); } if (!writeMutexCell(null, PhoenixDatabaseMetaData.SYSTEM_CATALOG_SCHEMA, PhoenixDatabaseMetaData.SYSTEM_CATALOG_TABLE, null, null)) { @@ -4294,11 +4295,9 @@ public boolean acquireUpgradeMutex(long currentServerSideTableTimestamp) public boolean writeMutexCell(String tenantId, String schemaName, String tableName, String columnName, String familyName) throws SQLException { try { - byte[] rowKey = - columnName != null - ? SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName, - familyName) - : SchemaUtil.getTableKey(tenantId, schemaName, tableName); + byte[] rowKey = columnName != null ? + SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName, familyName) : + SchemaUtil.getTableKey(tenantId, schemaName, tableName); // at this point the system mutex table should have been created or // an exception thrown byte[] sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes(); @@ -4338,11 +4337,9 @@ public void releaseUpgradeMutex() throws IOException, SQLException { public void deleteMutexCell(String tenantId, String schemaName, String tableName, String columnName, String familyName) throws SQLException { try { - byte[] rowKey = - columnName != null - ? SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName, - familyName) - : SchemaUtil.getTableKey(tenantId, schemaName, tableName); + byte[] rowKey = columnName != null ? + SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName, familyName) : + SchemaUtil.getTableKey(tenantId, schemaName, tableName); // at this point the system mutex table should have been created or // an exception thrown byte[] sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes(); @@ -4365,16 +4362,15 @@ 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; + try (Admin admin = getAdmin()) { + if (admin.tableExists(PhoenixDatabaseMetaData.SYSTEM_MUTEX_HBASE_TABLE_NAME)) { + return 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(); + SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName()))) { + return SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName(); } } - return sysMutexPhysicalTableNameBytes; + throw new TableNotFoundException(SYSTEM_SCHEMA_NAME, SYSTEM_MUTEX_TABLE_NAME); } private String addColumn(String columnsToAddSoFar, String columns) { From 9aafabf08960b698b38cb00088650cfeabeab9d3 Mon Sep 17 00:00:00 2001 From: Viraj Jasani Date: Wed, 14 Oct 2020 19:52:11 +0530 Subject: [PATCH 2/6] removing redundant call to getSysMutexPhysicalTableNameBytes() from acquireUpgradeMutex() --- .../query/ConnectionQueryServicesImpl.java | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java b/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java index a91087d28c1..826403fcdee 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java @@ -4277,12 +4277,6 @@ public boolean acquireUpgradeMutex(long currentServerSideTableTimestamp) throws IOException, SQLException { Preconditions.checkArgument(currentServerSideTableTimestamp < MIN_SYSTEM_TABLE_TIMESTAMP); - try { - getSysMutexPhysicalTableNameBytes(); - } catch (TableNotFoundException e) { - throw new UpgradeInProgressException(getVersion(currentServerSideTableTimestamp), - getVersion(MIN_SYSTEM_TABLE_TIMESTAMP)); - } if (!writeMutexCell(null, PhoenixDatabaseMetaData.SYSTEM_CATALOG_SCHEMA, PhoenixDatabaseMetaData.SYSTEM_CATALOG_TABLE, null, null)) { throw new UpgradeInProgressException(getVersion(currentServerSideTableTimestamp), @@ -4299,8 +4293,13 @@ public boolean writeMutexCell(String tenantId, String schemaName, String tableNa SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName, familyName) : SchemaUtil.getTableKey(tenantId, schemaName, tableName); // at this point the system mutex table should have been created or - // an exception thrown - byte[] sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes(); + // we should return false because without table, we cannot take a lock. + byte[] sysMutexPhysicalTableNameBytes; + try { + sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes(); + } catch (TableNotFoundException e) { + return false; + } try (Table sysMutexTable = getTable(sysMutexPhysicalTableNameBytes)) { byte[] family = PhoenixDatabaseMetaData.SYSTEM_MUTEX_FAMILY_NAME_BYTES; byte[] qualifier = PhoenixDatabaseMetaData.SYSTEM_MUTEX_COLUMN_NAME_BYTES; From 30e1a9eca2f528fb0380b98d1cdfa40b2fe4e6e8 Mon Sep 17 00:00:00 2001 From: Viraj Jasani Date: Wed, 14 Oct 2020 20:09:02 +0530 Subject: [PATCH 3/6] Revert "removing redundant call to getSysMutexPhysicalTableNameBytes() from acquireUpgradeMutex()" This reverts commit 9aafabf08960b698b38cb00088650cfeabeab9d3. --- .../query/ConnectionQueryServicesImpl.java | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java b/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java index 826403fcdee..a91087d28c1 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java @@ -4277,6 +4277,12 @@ public boolean acquireUpgradeMutex(long currentServerSideTableTimestamp) throws IOException, SQLException { Preconditions.checkArgument(currentServerSideTableTimestamp < MIN_SYSTEM_TABLE_TIMESTAMP); + try { + getSysMutexPhysicalTableNameBytes(); + } catch (TableNotFoundException e) { + throw new UpgradeInProgressException(getVersion(currentServerSideTableTimestamp), + getVersion(MIN_SYSTEM_TABLE_TIMESTAMP)); + } if (!writeMutexCell(null, PhoenixDatabaseMetaData.SYSTEM_CATALOG_SCHEMA, PhoenixDatabaseMetaData.SYSTEM_CATALOG_TABLE, null, null)) { throw new UpgradeInProgressException(getVersion(currentServerSideTableTimestamp), @@ -4293,13 +4299,8 @@ public boolean writeMutexCell(String tenantId, String schemaName, String tableNa SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName, familyName) : SchemaUtil.getTableKey(tenantId, schemaName, tableName); // at this point the system mutex table should have been created or - // we should return false because without table, we cannot take a lock. - byte[] sysMutexPhysicalTableNameBytes; - try { - sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes(); - } catch (TableNotFoundException e) { - return false; - } + // an exception thrown + byte[] sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes(); try (Table sysMutexTable = getTable(sysMutexPhysicalTableNameBytes)) { byte[] family = PhoenixDatabaseMetaData.SYSTEM_MUTEX_FAMILY_NAME_BYTES; byte[] qualifier = PhoenixDatabaseMetaData.SYSTEM_MUTEX_COLUMN_NAME_BYTES; From c0abcd6fa537ea708014cf8b904f62c4d65fdbcb Mon Sep 17 00:00:00 2001 From: Viraj Jasani Date: Thu, 15 Oct 2020 19:19:53 +0530 Subject: [PATCH 4/6] checkstyle fix --- .../phoenix/query/ConnectionQueryServicesImpl.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java b/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java index a91087d28c1..dd97ee13f02 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java @@ -4295,9 +4295,10 @@ public boolean acquireUpgradeMutex(long currentServerSideTableTimestamp) public boolean writeMutexCell(String tenantId, String schemaName, String tableName, String columnName, String familyName) throws SQLException { try { - byte[] rowKey = columnName != null ? - SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName, familyName) : - SchemaUtil.getTableKey(tenantId, schemaName, tableName); + byte[] rowKey = columnName != null + ? SchemaUtil.getColumnKey(tenantId, schemaName, tableName, + columnName, familyName) + : SchemaUtil.getTableKey(tenantId, schemaName, tableName); // at this point the system mutex table should have been created or // an exception thrown byte[] sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes(); @@ -4337,9 +4338,10 @@ public void releaseUpgradeMutex() throws IOException, SQLException { public void deleteMutexCell(String tenantId, String schemaName, String tableName, String columnName, String familyName) throws SQLException { try { - byte[] rowKey = columnName != null ? - SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName, familyName) : - SchemaUtil.getTableKey(tenantId, schemaName, tableName); + byte[] rowKey = columnName != null + ? SchemaUtil.getColumnKey(tenantId, schemaName, tableName, + columnName, familyName) + : SchemaUtil.getTableKey(tenantId, schemaName, tableName); // at this point the system mutex table should have been created or // an exception thrown byte[] sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes(); From da486863e993d0d4fc9b96519cb359b2a0c3d49c Mon Sep 17 00:00:00 2001 From: Viraj Jasani Date: Fri, 16 Oct 2020 11:38:08 +0530 Subject: [PATCH 5/6] incorporating review --- .../query/ConnectionQueryServicesImpl.java | 31 +++++++------------ 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java b/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java index dd97ee13f02..72e434f87d2 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java @@ -4269,20 +4269,12 @@ void ensureSystemTablesMigratedToSystemNamespace() * making use of HBase's checkAndPut api. * * @return true if client won the race, false otherwise - * @throws IOException * @throws SQLException */ @VisibleForTesting public boolean acquireUpgradeMutex(long currentServerSideTableTimestamp) - throws IOException, - SQLException { + throws SQLException { Preconditions.checkArgument(currentServerSideTableTimestamp < MIN_SYSTEM_TABLE_TIMESTAMP); - try { - getSysMutexPhysicalTableNameBytes(); - } catch (TableNotFoundException e) { - throw new UpgradeInProgressException(getVersion(currentServerSideTableTimestamp), - getVersion(MIN_SYSTEM_TABLE_TIMESTAMP)); - } if (!writeMutexCell(null, PhoenixDatabaseMetaData.SYSTEM_CATALOG_SCHEMA, PhoenixDatabaseMetaData.SYSTEM_CATALOG_TABLE, null, null)) { throw new UpgradeInProgressException(getVersion(currentServerSideTableTimestamp), @@ -4301,8 +4293,7 @@ public boolean writeMutexCell(String tenantId, String schemaName, String tableNa : SchemaUtil.getTableKey(tenantId, schemaName, tableName); // at this point the system mutex table should have been created or // an exception thrown - byte[] sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes(); - try (Table sysMutexTable = getTable(sysMutexPhysicalTableNameBytes)) { + try (Table sysMutexTable = getSysMutexTable()) { byte[] family = PhoenixDatabaseMetaData.SYSTEM_MUTEX_FAMILY_NAME_BYTES; byte[] qualifier = PhoenixDatabaseMetaData.SYSTEM_MUTEX_COLUMN_NAME_BYTES; byte[] value = MUTEX_LOCKED; @@ -4344,8 +4335,7 @@ public void deleteMutexCell(String tenantId, String schemaName, String tableName : SchemaUtil.getTableKey(tenantId, schemaName, tableName); // at this point the system mutex table should have been created or // an exception thrown - byte[] sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes(); - try (Table sysMutexTable = getTable(sysMutexPhysicalTableNameBytes)) { + try (Table sysMutexTable = getSysMutexTable()) { byte[] family = PhoenixDatabaseMetaData.SYSTEM_MUTEX_FAMILY_NAME_BYTES; byte[] qualifier = PhoenixDatabaseMetaData.SYSTEM_MUTEX_COLUMN_NAME_BYTES; Delete delete = new Delete(rowKey); @@ -4363,16 +4353,17 @@ public void deleteMutexCell(String tenantId, String schemaName, String tableName } } - private byte[] getSysMutexPhysicalTableNameBytes() throws IOException, SQLException { + private Table getSysMutexTable() throws SQLException, IOException { + String table = SYSTEM_MUTEX_NAME; + TableName tableName = TableName.valueOf(table); try (Admin admin = getAdmin()) { - if (admin.tableExists(PhoenixDatabaseMetaData.SYSTEM_MUTEX_HBASE_TABLE_NAME)) { - return PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME_BYTES; - } else if (admin.tableExists(TableName.valueOf( - SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName()))) { - return SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName(); + if (!admin.tableExists(tableName)) { + table = table.replace(QueryConstants.NAME_SEPARATOR, + QueryConstants.NAMESPACE_SEPARATOR); + tableName = TableName.valueOf(table); } + return connection.getTable(tableName); } - throw new TableNotFoundException(SYSTEM_SCHEMA_NAME, SYSTEM_MUTEX_TABLE_NAME); } private String addColumn(String columnsToAddSoFar, String columns) { From d341b40d5336a1bc7c14b704590683e025f839df Mon Sep 17 00:00:00 2001 From: Viraj Jasani Date: Thu, 22 Oct 2020 01:03:21 +0530 Subject: [PATCH 6/6] addressing review --- .../query/ConnectionQueryServicesImpl.java | 3 +- .../ConnectionQueryServicesImplTest.java | 40 +++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java b/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java index cab860e11d0..d0bd6ed21e8 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java @@ -4443,7 +4443,8 @@ public void deleteMutexCell(String tenantId, String schemaName, String tableName } } - private Table getSysMutexTable() throws SQLException, IOException { + @VisibleForTesting + public Table getSysMutexTable() throws SQLException, IOException { String table = SYSTEM_MUTEX_NAME; TableName tableName = TableName.valueOf(table); try (Admin admin = getAdmin()) { diff --git a/phoenix-core/src/test/java/org/apache/phoenix/query/ConnectionQueryServicesImplTest.java b/phoenix-core/src/test/java/org/apache/phoenix/query/ConnectionQueryServicesImplTest.java index 08e7f91c8ed..205267696bd 100644 --- a/phoenix-core/src/test/java/org/apache/phoenix/query/ConnectionQueryServicesImplTest.java +++ b/phoenix-core/src/test/java/org/apache/phoenix/query/ConnectionQueryServicesImplTest.java @@ -23,6 +23,7 @@ import static org.apache.phoenix.jdbc.PhoenixDatabaseMetaData.TTL_FOR_MUTEX; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.Matchers.any; @@ -46,7 +47,9 @@ import org.apache.hadoop.hbase.client.Admin; import org.apache.hadoop.hbase.TableNotFoundException; import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder; +import org.apache.hadoop.hbase.client.Connection; import org.apache.hadoop.hbase.client.Mutation; +import org.apache.hadoop.hbase.client.Table; import org.apache.hadoop.hbase.client.TableDescriptor; import org.apache.hadoop.hbase.client.TableDescriptorBuilder; import org.apache.phoenix.exception.PhoenixIOException; @@ -78,6 +81,12 @@ public class ConnectionQueryServicesImplTest { @Mock private ReadOnlyProps readOnlyProps; + @Mock + private Connection mockConn; + + @Mock + private Table mockTable; + public static final TableDescriptorBuilder SYS_TASK_TDB = TableDescriptorBuilder .newBuilder(TableName.valueOf(PhoenixDatabaseMetaData.SYSTEM_TASK_NAME)); public static final TableDescriptorBuilder SYS_TASK_TDB_SP = TableDescriptorBuilder @@ -93,12 +102,17 @@ public void setup() throws IOException, NoSuchFieldException, .getDeclaredField("props"); props.setAccessible(true); props.set(mockCqs, readOnlyProps); + props = ConnectionQueryServicesImpl.class.getDeclaredField("connection"); + props.setAccessible(true); + props.set(mockCqs, mockConn); when(mockCqs.checkIfSysMutexExistsAndModifyTTLIfRequired(mockAdmin)) .thenCallRealMethod(); when(mockCqs.updateAndConfirmSplitPolicyForTask(SYS_TASK_TDB)) .thenCallRealMethod(); when(mockCqs.updateAndConfirmSplitPolicyForTask(SYS_TASK_TDB_SP)) .thenCallRealMethod(); + when(mockCqs.getSysMutexTable()).thenCallRealMethod(); + when(mockCqs.getAdmin()).thenCallRealMethod(); } @SuppressWarnings("unchecked") @@ -193,4 +207,30 @@ public void testSysTaskSplitPolicyWithError() { e.getMessage()); } } + + @Test + public void testGetSysMutexTableWithName() throws Exception { + when(mockAdmin.tableExists(any())).thenReturn(true); + when(mockConn.getAdmin()).thenReturn(mockAdmin); + when(mockConn.getTable(TableName.valueOf("SYSTEM.MUTEX"))) + .thenReturn(mockTable); + assertSame(mockCqs.getSysMutexTable(), mockTable); + verify(mockAdmin, Mockito.times(1)).tableExists(any()); + verify(mockConn, Mockito.times(1)).getAdmin(); + verify(mockConn, Mockito.times(1)) + .getTable(TableName.valueOf("SYSTEM.MUTEX")); + } + + @Test + public void testGetSysMutexTableWithNamespace() throws Exception { + when(mockAdmin.tableExists(any())).thenReturn(false); + when(mockConn.getAdmin()).thenReturn(mockAdmin); + when(mockConn.getTable(TableName.valueOf("SYSTEM:MUTEX"))) + .thenReturn(mockTable); + assertSame(mockCqs.getSysMutexTable(), mockTable); + verify(mockAdmin, Mockito.times(1)).tableExists(any()); + verify(mockConn, Mockito.times(1)).getAdmin(); + verify(mockConn, Mockito.times(1)) + .getTable(TableName.valueOf("SYSTEM:MUTEX")); + } }