diff --git a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/ApiaryGlueSync.java b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/ApiaryGlueSync.java index 7681b395..60a144d6 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/ApiaryGlueSync.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/ApiaryGlueSync.java @@ -186,15 +186,9 @@ public void onDropTable(DropTableEvent event) throws MetaException { } Table table = event.getTable(); try { - glueTableService.deleteIfUnchanged(table); + GlueTableService.DeleteOutcome outcome = glueTableService.deleteIfUnchanged(table); metricService.incrementCounter(MetricConstants.LISTENER_TABLE_SUCCESS); - metricService.recordEvent(MetricConstants.DROP_TABLE, MetricConstants.RESULT_SUCCESS, "deleted"); - } catch (EntityNotFoundException e) { - log.info(table + " table doesn't exist in glue catalog"); - metricService.recordEvent(MetricConstants.DROP_TABLE, MetricConstants.RESULT_SUCCESS, "not_found"); - if (throwExceptions) { - throw wrap(e); - } + metricService.recordEvent(MetricConstants.DROP_TABLE, MetricConstants.RESULT_SUCCESS, outcome.metricOutcome()); } catch (Exception e) { log.error("Failed drop table {}.{} in glue", table.getDbName(), table.getTableName(), e); metricService.incrementCounter(MetricConstants.LISTENER_TABLE_FAILURE); @@ -293,10 +287,10 @@ private void doRenameOperation(Table oldTable, Table newTable) { long startTime = System.currentTimeMillis(); glueTableService.create(newTable); gluePartitionService.copyPartitions(newTable, gluePartitionService.getPartitions(oldTable)); - glueTableService.deleteIfUnchanged(oldTable); - metricService.incrementCounter(MetricConstants.LISTENER_TABLE_SUCCESS); - metricService.recordEvent(MetricConstants.ALTER_TABLE, MetricConstants.RESULT_SUCCESS, "renamed"); + GlueTableService.DeleteOutcome outcome = glueTableService.deleteIfUnchanged(oldTable); long duration = System.currentTimeMillis() - startTime; + metricService.incrementCounter(MetricConstants.LISTENER_TABLE_SUCCESS); + metricService.recordEvent(MetricConstants.RENAME_TABLE, MetricConstants.RESULT_SUCCESS, outcome.metricOutcome()); metricService.recordDuration(MetricConstants.LISTENER_TABLE_RENAME_DURATION, duration); log.info("{} glue table rename to {} finished in {}ms", oldTable.getTableName(), newTable.getTableName(), duration); } diff --git a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricConstants.java b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricConstants.java index 618181a7..f56cccb8 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricConstants.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricConstants.java @@ -46,6 +46,7 @@ public class MetricConstants { public static final String CREATE_TABLE = "create_table"; public static final String DROP_TABLE = "drop_table"; public static final String ALTER_TABLE = "alter_table"; + public static final String RENAME_TABLE = "rename_table"; public static final String ADD_PARTITION = "add_partition"; public static final String DROP_PARTITION = "drop_partition"; public static final String ALTER_PARTITION = "alter_partition"; diff --git a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/service/GlueTableService.java b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/service/GlueTableService.java index 307c657c..de679fdd 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/service/GlueTableService.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/service/GlueTableService.java @@ -34,6 +34,35 @@ import com.amazonaws.services.glue.model.ValidationException; public class GlueTableService { + + /** + * Outcome of a {@link #deleteIfUnchanged} call, used by callers to record the appropriate metric. + * + */ + public enum DeleteOutcome { + DELETED("deleted"), + SKIPPED("delete_skipped"), + NOT_FOUND("not_found"), + DELETED_CONCURRENTLY("concurrent_delete"); + + private final String metricOutcome; + + DeleteOutcome(String metricOutcome) { + this.metricOutcome = metricOutcome; + } + + public String metricOutcome() { + return metricOutcome; + } + } + private static final Logger log = LoggerFactory.getLogger(GlueTableService.class); // HMS parameters checked before a delete to detect if the table slot was overwritten by a concurrent operation private static final String[] IDENTITY_PARAMS = { "transient_lastDdlTime", "metadata_location" }; @@ -112,7 +141,7 @@ public void delete(Table table) { * delete will be skipped, leaving an orphan in Glue. An orphan is intentionally preferred over * incorrectly deleting a table that has since been overwritten by a rename. */ - public void deleteIfUnchanged(Table table) { + public DeleteOutcome deleteIfUnchanged(Table table) { Map hmsParams = table.getParameters() != null ? table.getParameters() : Collections.emptyMap(); com.amazonaws.services.glue.model.Table glueTable; try { @@ -123,17 +152,24 @@ public void deleteIfUnchanged(Table table) { .getTable(); } catch (EntityNotFoundException e) { log.info("{}.{} table not found in glue catalog, nothing to delete", table.getDbName(), table.getTableName()); - return; + return DeleteOutcome.NOT_FOUND; } Map glueParams = glueTable.getParameters() != null ? glueTable.getParameters() : Collections.emptyMap(); for (String key : IDENTITY_PARAMS) { if (paramChanged(key, hmsParams, glueParams, table)) { - return; + return DeleteOutcome.SKIPPED; } } - delete(table); + try { + delete(table); + } catch (EntityNotFoundException e) { + log.info("{}.{} table deleted from glue catalog between guard check and delete", + table.getDbName(), table.getTableName()); + return DeleteOutcome.DELETED_CONCURRENTLY; + } + return DeleteOutcome.DELETED; } private boolean paramChanged(String key, Map hmsParams, Map glueParams, Table table) { diff --git a/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/ApiaryGlueSyncTest.java b/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/ApiaryGlueSyncTest.java index 458ca2c5..b2e1fe2a 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/ApiaryGlueSyncTest.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/ApiaryGlueSyncTest.java @@ -482,7 +482,7 @@ public void onAlterHiveTable_RenameTable() throws MetaException { BatchCreatePartitionRequest batchCreatePartitionRequest = batchCreatePartitionRequestCaptor.getValue(); verify(glueClient).deleteTable(deleteTableRequestCaptor.capture()); verify(metricService).incrementCounter(MetricConstants.LISTENER_TABLE_SUCCESS); - verify(metricService).recordEvent(MetricConstants.ALTER_TABLE, MetricConstants.RESULT_SUCCESS, "renamed"); + verify(metricService).recordEvent(MetricConstants.RENAME_TABLE, MetricConstants.RESULT_SUCCESS, "deleted"); verify(metricService).recordDuration(eq(MetricConstants.LISTENER_TABLE_RENAME_DURATION), anyLong()); DeleteTableRequest deleteTableRequest = deleteTableRequestCaptor.getValue(); @@ -721,16 +721,63 @@ public void onDropTableThatDoesntExistInGlue() throws MetaException { DropTableEvent event = mock(DropTableEvent.class); when(event.getStatus()).thenReturn(true); when(event.getTable()).thenReturn(simpleHiveTable(simpleSchema(), simplePartitioning())); - when(glueClient.getTable(any())).thenReturn(new GetTableResult().withTable(new com.amazonaws.services.glue.model.Table())); - when(glueClient.deleteTable(any())).thenThrow(new EntityNotFoundException("")); + when(glueClient.getTable(any())).thenThrow(new EntityNotFoundException("")); glueSync.onDropTable(event); - verify(glueClient).deleteTable(any()); + verify(glueClient, never()).deleteTable(any()); + verify(metricService).incrementCounter(MetricConstants.LISTENER_TABLE_SUCCESS); verify(metricService).recordEvent(MetricConstants.DROP_TABLE, MetricConstants.RESULT_SUCCESS, "not_found"); verifyNoMoreInteractions(metricService); } + @Test + public void onDropTable_skipsDeleteWhenGuardFires() throws MetaException { + DropTableEvent event = mock(DropTableEvent.class); + when(event.getStatus()).thenReturn(true); + Table hmsTable = simpleHiveTable(simpleSchema(), simplePartitioning()); + hmsTable.putToParameters("transient_lastDdlTime", "100"); + when(event.getTable()).thenReturn(hmsTable); + when(glueClient.getTable(any())).thenReturn( + new GetTableResult().withTable(new com.amazonaws.services.glue.model.Table() + .withParameters(ImmutableMap.of("transient_lastDdlTime", "999")))); + + glueSync.onDropTable(event); + + verify(glueClient, never()).deleteTable(any()); + verify(metricService).incrementCounter(MetricConstants.LISTENER_TABLE_SUCCESS); + verify(metricService).recordEvent(MetricConstants.DROP_TABLE, MetricConstants.RESULT_SUCCESS, "delete_skipped"); + verifyNoMoreInteractions(metricService); + } + + @Test + public void onAlterHiveTable_RenameTable_skipsOldTableDeleteWhenGuardFires() throws MetaException { + AlterTableEvent event = mock(AlterTableEvent.class); + when(event.getStatus()).thenReturn(true); + + Table oldTable = simpleHiveTable(simpleSchema(), simplePartitioning()); + oldTable.setTableName("table2"); + oldTable.putToParameters("transient_lastDdlTime", "100"); + when(event.getOldTable()).thenReturn(oldTable); + + Table newTable = simpleHiveTable(simpleSchema(), simplePartitioning()); + newTable.setTableName("table2_new"); + when(event.getNewTable()).thenReturn(newTable); + + when(glueClient.getPartitions(any())).thenReturn(new GetPartitionsResult().withPartitions(Collections.emptyList())); + // Glue has a different DDL time — guard fires, old table slot not deleted + when(glueClient.getTable(any())).thenReturn( + new GetTableResult().withTable(new com.amazonaws.services.glue.model.Table() + .withParameters(ImmutableMap.of("transient_lastDdlTime", "999")))); + + glueSync.onAlterTable(event); + + verify(glueClient, never()).deleteTable(any()); + verify(metricService).incrementCounter(MetricConstants.LISTENER_TABLE_SUCCESS); + verify(metricService).recordEvent(MetricConstants.RENAME_TABLE, MetricConstants.RESULT_SUCCESS, "delete_skipped"); + verify(metricService).recordDuration(eq(MetricConstants.LISTENER_TABLE_RENAME_DURATION), anyLong()); + } + @Test public void onAlterHiveTable_RenameOperationFailureIsMetered() throws MetaException { AlterTableEvent event = mock(AlterTableEvent.class); diff --git a/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/service/GlueTableServiceTest.java b/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/service/GlueTableServiceTest.java index 4cd5f014..7c4dd25c 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/service/GlueTableServiceTest.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/service/GlueTableServiceTest.java @@ -15,6 +15,8 @@ */ package com.expediagroup.apiary.extensions.gluesync.listener.service; +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.MatcherAssert.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; @@ -62,8 +64,9 @@ public void setUp() { public void deleteIfUnchanged_deletesWhenAllPropertiesMatch() { when(glueClient.getTable(any(GetTableRequest.class))).thenReturn(glueTableResult(LOCATION, LAST_DDL_TIME, null)); - service.deleteIfUnchanged(hmsTable(LOCATION, LAST_DDL_TIME, null)); + GlueTableService.DeleteOutcome outcome = service.deleteIfUnchanged(hmsTable(LOCATION, LAST_DDL_TIME, null)); + assertThat(outcome, is(GlueTableService.DeleteOutcome.DELETED)); verify(glueClient).deleteTable(any(DeleteTableRequest.class)); } @@ -71,8 +74,9 @@ public void deleteIfUnchanged_deletesWhenAllPropertiesMatch() { public void deleteIfUnchanged_skipsWhenLastDdlTimeChanged() { when(glueClient.getTable(any(GetTableRequest.class))).thenReturn(glueTableResult(LOCATION, "1751385999", null)); - service.deleteIfUnchanged(hmsTable(LOCATION, LAST_DDL_TIME, null)); + GlueTableService.DeleteOutcome outcome = service.deleteIfUnchanged(hmsTable(LOCATION, LAST_DDL_TIME, null)); + assertThat(outcome, is(GlueTableService.DeleteOutcome.SKIPPED)); verify(glueClient, never()).deleteTable(any(DeleteTableRequest.class)); } @@ -82,8 +86,9 @@ public void deleteIfUnchanged_skipsWhenHiveTableReplacedByIceberg() { when(glueClient.getTable(any(GetTableRequest.class))) .thenReturn(glueTableResult(LOCATION, LAST_DDL_TIME, "s3://bucket/test_table/metadata/v1.metadata.json")); - service.deleteIfUnchanged(hmsTable(LOCATION, LAST_DDL_TIME, null)); + GlueTableService.DeleteOutcome outcome = service.deleteIfUnchanged(hmsTable(LOCATION, LAST_DDL_TIME, null)); + assertThat(outcome, is(GlueTableService.DeleteOutcome.SKIPPED)); verify(glueClient, never()).deleteTable(any(DeleteTableRequest.class)); } @@ -92,8 +97,10 @@ public void deleteIfUnchanged_skipsWhenMetadataLocationDiverges() { when(glueClient.getTable(any(GetTableRequest.class))) .thenReturn(glueTableResult(LOCATION, LAST_DDL_TIME, "s3://bucket/test_table/metadata/v2.metadata.json")); - service.deleteIfUnchanged(hmsTable(LOCATION, LAST_DDL_TIME, "s3://bucket/test_table/metadata/v1.metadata.json")); + GlueTableService.DeleteOutcome outcome = service.deleteIfUnchanged( + hmsTable(LOCATION, LAST_DDL_TIME, "s3://bucket/test_table/metadata/v1.metadata.json")); + assertThat(outcome, is(GlueTableService.DeleteOutcome.SKIPPED)); verify(glueClient, never()).deleteTable(any(DeleteTableRequest.class)); } @@ -101,28 +108,42 @@ public void deleteIfUnchanged_skipsWhenMetadataLocationDiverges() { public void deleteIfUnchanged_skipsWhenHmsHasMetadataLocationButGlueDoesNot() { when(glueClient.getTable(any(GetTableRequest.class))).thenReturn(glueTableResult(LOCATION, LAST_DDL_TIME, null)); - service.deleteIfUnchanged(hmsTable(LOCATION, LAST_DDL_TIME, "s3://bucket/test_table/metadata/v1.metadata.json")); + GlueTableService.DeleteOutcome outcome = service.deleteIfUnchanged( + hmsTable(LOCATION, LAST_DDL_TIME, "s3://bucket/test_table/metadata/v1.metadata.json")); + assertThat(outcome, is(GlueTableService.DeleteOutcome.SKIPPED)); verify(glueClient, never()).deleteTable(any(DeleteTableRequest.class)); } @Test - public void deleteIfUnchanged_skipsWhenTableNotFoundInGlue() { + public void deleteIfUnchanged_notFoundWhenTableAbsentDuringGet() { when(glueClient.getTable(any(GetTableRequest.class))).thenThrow(new EntityNotFoundException("not found")); - service.deleteIfUnchanged(hmsTable(LOCATION, LAST_DDL_TIME, null)); + GlueTableService.DeleteOutcome outcome = service.deleteIfUnchanged(hmsTable(LOCATION, LAST_DDL_TIME, null)); + assertThat(outcome, is(GlueTableService.DeleteOutcome.NOT_FOUND)); verify(glueClient, never()).deleteTable(any(DeleteTableRequest.class)); } + @Test + public void deleteIfUnchanged_deletedConcurrentlyWhenTableDeletedBetweenGuardAndDelete() { + when(glueClient.getTable(any(GetTableRequest.class))).thenReturn(glueTableResult(LOCATION, LAST_DDL_TIME, null)); + when(glueClient.deleteTable(any(DeleteTableRequest.class))).thenThrow(new EntityNotFoundException("not found")); + + GlueTableService.DeleteOutcome outcome = service.deleteIfUnchanged(hmsTable(LOCATION, LAST_DDL_TIME, null)); + + assertThat(outcome, is(GlueTableService.DeleteOutcome.DELETED_CONCURRENTLY)); + } + @Test public void deleteIfUnchanged_deletesWhenBothParamMapsAreNull() { when(glueClient.getTable(any(GetTableRequest.class))).thenReturn(glueTableResultNullParams()); Table table = hmsTable(LOCATION, null, null); table.setParameters(null); - service.deleteIfUnchanged(table); + GlueTableService.DeleteOutcome outcome = service.deleteIfUnchanged(table); + assertThat(outcome, is(GlueTableService.DeleteOutcome.DELETED)); verify(glueClient).deleteTable(any(DeleteTableRequest.class)); } @@ -132,8 +153,9 @@ public void deleteIfUnchanged_skipsWhenHmsParamsNullButGlueHasParams() { Table table = hmsTable(LOCATION, null, null); table.setParameters(null); - service.deleteIfUnchanged(table); + GlueTableService.DeleteOutcome outcome = service.deleteIfUnchanged(table); + assertThat(outcome, is(GlueTableService.DeleteOutcome.SKIPPED)); verify(glueClient, never()).deleteTable(any(DeleteTableRequest.class)); } @@ -141,8 +163,9 @@ public void deleteIfUnchanged_skipsWhenHmsParamsNullButGlueHasParams() { public void deleteIfUnchanged_skipsWhenGlueParamsNullButHmsHasParams() { when(glueClient.getTable(any(GetTableRequest.class))).thenReturn(glueTableResultNullParams()); - service.deleteIfUnchanged(hmsTable(LOCATION, LAST_DDL_TIME, null)); + GlueTableService.DeleteOutcome outcome = service.deleteIfUnchanged(hmsTable(LOCATION, LAST_DDL_TIME, null)); + assertThat(outcome, is(GlueTableService.DeleteOutcome.SKIPPED)); verify(glueClient, never()).deleteTable(any(DeleteTableRequest.class)); }