Skip to content

Commit

Permalink
PHOENIX-7449 Handling already disabled table gracefully during dropTa…
Browse files Browse the repository at this point in the history 
…bles execution (#2018)

* PHOENIX-7449 Handling already disabled table gracefully during dropTables execution
  • Loading branch information
@ritegarg
ritegarg authored Oct 29, 2024
1 parent 46c4735 commit aad6208
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 7 deletions.
22 changes: 16 additions & 6 deletions phoenix-core-client/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@
import org.apache.hadoop.hbase.ServerName;
import org.apache.hadoop.hbase.TableExistsException;
import org.apache.hadoop.hbase.TableName;
import org.apache.hadoop.hbase.TableNotEnabledException;
import org.apache.hadoop.hbase.client.Admin;
import org.apache.hadoop.hbase.client.Append;
import org.apache.hadoop.hbase.client.CheckAndMutate;
Expand Down Expand Up @@ -1790,7 +1791,7 @@ private TableDescriptor ensureTableCreated(byte[] physicalTableName, byte[] pare
SQLExceptionCode.INCONSISTENT_NAMESPACE_MAPPING_PROPERTIES.getErrorCode()) {
try {
// In case we wrongly created SYSTEM.CATALOG or SYSTEM:CATALOG, we should drop it
admin.disableTable(TableName.valueOf(physicalTableName));
disableTable(admin, TableName.valueOf(physicalTableName));
admin.deleteTable(TableName.valueOf(physicalTableName));
} catch (org.apache.hadoop.hbase.TableNotFoundException ignored) {
// Ignore this since it just means that another client with a similar set of
Expand Down Expand Up @@ -1946,7 +1947,7 @@ private void modifyTable(byte[] tableName, TableDescriptor newDesc, boolean shou
TableName tn = TableName.valueOf(tableName);
try (Admin admin = getAdmin()) {
if (!allowOnlineTableSchemaUpdate()) {
admin.disableTable(tn);
disableTable(admin, tn);
admin.modifyTable(newDesc); // TODO: Update to TableDescriptor
admin.enableTable(tn);
} else {
Expand Down Expand Up @@ -2239,6 +2240,14 @@ private void ensureViewIndexTableCreated(byte[] physicalTableName, byte[] parent
}
}

private void disableTable(Admin admin, TableName tableName) throws IOException {
try {
admin.disableTable(tableName);
} catch (TableNotEnabledException e) {
LOGGER.info("Table already disabled, continuing with next steps", e);
}
}

private boolean ensureViewIndexTableDropped(byte[] physicalTableName, long timestamp) throws SQLException {
byte[] physicalIndexName = MetaDataUtil.getViewIndexPhysicalName(physicalTableName);
boolean wasDeleted = false;
Expand All @@ -2250,7 +2259,7 @@ private boolean ensureViewIndexTableDropped(byte[] physicalTableName, long times
final ReadOnlyProps props = this.getProps();
final boolean dropMetadata = props.getBoolean(DROP_METADATA_ATTRIB, DEFAULT_DROP_METADATA);
if (dropMetadata) {
admin.disableTable(physicalIndexTableName);
disableTable(admin, physicalIndexTableName);
admin.deleteTable(physicalIndexTableName);
clearTableRegionCache(physicalIndexTableName);
wasDeleted = true;
Expand Down Expand Up @@ -2583,15 +2592,16 @@ private void dropTable(byte[] tableNameToDelete) throws SQLException {
dropTables(Collections.<byte[]>singletonList(tableNameToDelete));
}

private void dropTables(final List<byte[]> tableNamesToDelete) throws SQLException {
@VisibleForTesting
void dropTables(final List<byte[]> tableNamesToDelete) throws SQLException {
SQLException sqlE = null;
try (Admin admin = getAdmin()) {
if (tableNamesToDelete != null) {
for ( byte[] tableName : tableNamesToDelete ) {
try {
TableName tn = TableName.valueOf(tableName);
TableDescriptor htableDesc = this.getTableDescriptor(tableName);
admin.disableTable(tn);
disableTable(admin, tn);
admin.deleteTable(tn);
tableStatsCache.invalidateAll(htableDesc);
clearTableRegionCache(TableName.valueOf(tableName));
Expand Down Expand Up @@ -4066,7 +4076,7 @@ protected PhoenixConnection upgradeSystemCatalogIfRequired(PhoenixConnection met
// co-location of data and index regions. If we just modify the
// table descriptor when online schema change enabled may reopen
// the region in same region server instead of following data region.
admin.disableTable(table.getTableName());
disableTable(admin, table.getTableName());
admin.modifyTable(table);
admin.enableTable(table.getTableName());
}
Expand Down
33 changes: 32 additions & 1 deletion phoenix-core/src/test/java/org/apache/phoenix/query/ConnectionQueryServicesImplTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@

import java.io.IOException;
import java.lang.reflect.Field;
import java.nio.charset.StandardCharsets;
import java.sql.SQLException;
import java.util.Collections;
import java.util.HashMap;
Expand All @@ -45,6 +46,7 @@
import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.TableName;
import org.apache.hadoop.hbase.client.Admin;
import org.apache.hadoop.hbase.TableNotEnabledException;
import org.apache.hadoop.hbase.TableNotFoundException;
import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder;
import org.apache.hadoop.hbase.client.Connection;
Expand All @@ -61,7 +63,6 @@
import org.apache.phoenix.util.ReadOnlyProps;
import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Ignore;
import org.junit.Test;
import org.mockito.Mock;
import org.mockito.Mockito;
Expand Down Expand Up @@ -96,6 +97,9 @@ public class ConnectionQueryServicesImplTest {
@Mock
private Table mockTable;

@Mock
private GuidePostsCacheWrapper mockTableStatsCache;

public static final TableDescriptorBuilder SYS_TASK_TDB = TableDescriptorBuilder
.newBuilder(TableName.valueOf(PhoenixDatabaseMetaData.SYSTEM_TASK_NAME));
public static final TableDescriptorBuilder SYS_TASK_TDB_SP = TableDescriptorBuilder
Expand All @@ -114,6 +118,9 @@ public void setup() throws IOException, NoSuchFieldException,
props = ConnectionQueryServicesImpl.class.getDeclaredField("connection");
props.setAccessible(true);
props.set(mockCqs, mockConn);
props = ConnectionQueryServicesImpl.class.getDeclaredField("tableStatsCache");
props.setAccessible(true);
props.set(mockCqs, mockTableStatsCache);
when(mockCqs.checkIfSysMutexExistsAndModifyTTLIfRequired(mockAdmin))
.thenCallRealMethod();
when(mockCqs.updateAndConfirmSplitPolicyForTask(SYS_TASK_TDB))
Expand All @@ -124,6 +131,7 @@ public void setup() throws IOException, NoSuchFieldException,
when(mockCqs.getAdmin()).thenCallRealMethod();
when(mockCqs.getTable(Mockito.any())).thenCallRealMethod();
when(mockCqs.getTableIfExists(Mockito.any())).thenCallRealMethod();
doCallRealMethod().when(mockCqs).dropTables(Mockito.any());
}

@SuppressWarnings("unchecked")
Expand Down Expand Up @@ -343,4 +351,27 @@ public void testGetSysMutexTableWithNamespace() throws Exception {
verify(mockConn, Mockito.times(1))
.getTable(TableName.valueOf("SYSTEM:MUTEX"));
}

@Test
public void testDropTablesAlreadyDisabled() throws Exception {
when(mockConn.getAdmin()).thenReturn(mockAdmin);
doThrow(new TableNotEnabledException()).when(mockAdmin).disableTable(any());
doNothing().when(mockAdmin).deleteTable(any());
mockCqs.dropTables(Collections.singletonList("TEST_TABLE".getBytes(StandardCharsets.UTF_8)));
verify(mockAdmin, Mockito.times(1)).disableTable(TableName.valueOf("TEST_TABLE"));
verify(mockAdmin, Mockito.times(1)).deleteTable(TableName.valueOf("TEST_TABLE"));
verify(mockConn).getAdmin();
}

@Test
public void testDropTablesTableEnabled() throws Exception {
when(mockConn.getAdmin()).thenReturn(mockAdmin);
doNothing().when(mockAdmin).disableTable(any());
doNothing().when(mockAdmin).deleteTable(any());
doNothing().when(mockTableStatsCache).invalidateAll();
mockCqs.dropTables(Collections.singletonList("TEST_TABLE".getBytes(StandardCharsets.UTF_8)));
verify(mockAdmin, Mockito.times(1)).disableTable(TableName.valueOf("TEST_TABLE"));
verify(mockAdmin, Mockito.times(1)).deleteTable(TableName.valueOf("TEST_TABLE"));
verify(mockConn).getAdmin();
}
}

0 comments on commit aad6208

Please sign in to comment.