This is an automated email from the ASF dual-hosted git repository. skadam pushed a commit to branch PHOENIX-6387-4.x in repository https://gitbox.apache.org/repos/asf/phoenix.git
commit 6a78f8399b34ba59b34bee9b95d97fd5588580aa Author: ankitjain64 <34427442+ankitjai...@users.noreply.github.com> AuthorDate: Wed May 5 16:44:45 2021 -0700 PHOENIX-6437: Parent-Child Delete marker should get replicated via Sy… (#1219) * PHOENIX-6437: Parent-Child Delete marker should get replicated via SystemCatalogWALEntryFilter Co-authored-by: Ankit Jain <jainan...@salesforce.com> --- .../replication/SystemCatalogWALEntryFilterIT.java | 194 +++++++++++++++++---- .../replication/SystemCatalogWALEntryFilter.java | 19 +- 2 files changed, 173 insertions(+), 40 deletions(-) diff --git a/phoenix-core/src/it/java/org/apache/phoenix/replication/SystemCatalogWALEntryFilterIT.java b/phoenix-core/src/it/java/org/apache/phoenix/replication/SystemCatalogWALEntryFilterIT.java index ab6af25..24d4990 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/replication/SystemCatalogWALEntryFilterIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/replication/SystemCatalogWALEntryFilterIT.java @@ -22,12 +22,14 @@ import org.apache.hadoop.hbase.CellUtil; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.client.*; import org.apache.hadoop.hbase.io.ImmutableBytesWritable; +import org.apache.hadoop.hbase.regionserver.wal.HLogKey; import org.apache.hadoop.hbase.replication.ChainWALEntryFilter; import org.apache.hadoop.hbase.regionserver.wal.WALEdit; import org.apache.hadoop.hbase.util.Bytes; import org.apache.hadoop.hbase.wal.WAL; import org.apache.hadoop.hbase.wal.WALKey; import org.apache.phoenix.end2end.ParallelStatsDisabledIT; +import org.apache.phoenix.hbase.index.wal.IndexedKeyValue; import org.apache.phoenix.jdbc.PhoenixDatabaseMetaData; import org.apache.phoenix.mapreduce.util.ConnectionUtil; import org.apache.phoenix.schema.PTable; @@ -67,8 +69,8 @@ public class SystemCatalogWALEntryFilterIT extends ParallelStatsDisabledIT { + NONTENANT_VIEW_NAME + "(" + VIEW_COLUMN_NAME + " varchar) AS SELECT * FROM " + TestUtil.ENTITY_HISTORY_TABLE_NAME + " WHERE OLD_VALUE like 'E%'"; - private static final String DROP_TENANT_VIEW_SQL = "DROP VIEW IF EXISTS " + TENANT_VIEW_NAME; - private static final String DROP_NONTENANT_VIEW_SQL = "DROP VIEW IF EXISTS " + NONTENANT_VIEW_NAME; + private static final String DROP_TENANT_VIEW_SQL = "DROP VIEW IF EXISTS " + SCHEMA_NAME + "." + TENANT_VIEW_NAME; + private static final String DROP_NONTENANT_VIEW_SQL = "DROP VIEW IF EXISTS " + SCHEMA_NAME + "." + NONTENANT_VIEW_NAME; private static PTable catalogTable; private static PTable childLinkTable; private static WALKey walKeyCatalog = null; @@ -97,24 +99,13 @@ public class SystemCatalogWALEntryFilterIT extends ParallelStatsDisabledIT { PhoenixDatabaseMetaData.SYSTEM_CHILD_LINK_NAME), 0, 0, uuid); }; Assert.assertNotNull(catalogTable); - try (java.sql.Connection connection = - ConnectionUtil.getInputConnection(getUtility().getConfiguration(), new Properties())) { - connection.createStatement().execute(CREATE_NONTENANT_VIEW_SQL); - }; + createNonTenantView(); } @AfterClass public static synchronized void tearDown() throws Exception { - Properties tenantProperties = new Properties(); - tenantProperties.setProperty("TenantId", TENANT_ID); - try (java.sql.Connection connection = - ConnectionUtil.getInputConnection(getUtility().getConfiguration(), tenantProperties)) { - connection.createStatement().execute(DROP_TENANT_VIEW_SQL); - } - try (java.sql.Connection connection = - ConnectionUtil.getInputConnection(getUtility().getConfiguration(), new Properties())) { - connection.createStatement().execute(DROP_NONTENANT_VIEW_SQL); - } + dropTenantView(); + dropNonTenantView(); } @Test @@ -136,7 +127,7 @@ public class SystemCatalogWALEntryFilterIT extends ParallelStatsDisabledIT { WAL.Entry nonTenantEntryCatalog = getEntry(systemCatalogTableName, nonTenantGetCatalog); WAL.Entry tenantEntryCatalog = getEntry(systemCatalogTableName, tenantGetCatalog); - int tenantRowCount = getAndAssertTenantCountInEdit(tenantEntryCatalog); + int tenantRowCount = getAndAssertCountInEdit(tenantEntryCatalog, true); Assert.assertTrue(tenantRowCount > 0); //verify that the tenant view WAL.Entry passes the filter and the non-tenant view does not @@ -151,9 +142,9 @@ public class SystemCatalogWALEntryFilterIT extends ParallelStatsDisabledIT { WAL.Entry filteredTenantEntryCatalog = chainWALEntryFilter.filter(tenantEntryCatalog); Assert.assertNotNull("Tenant view was filtered when it shouldn't be!", - filteredTenantEntryCatalog); + filteredTenantEntryCatalog); Assert.assertEquals("Not all data for replicated for tenant", tenantRowCount, - getAndAssertTenantCountInEdit(filteredTenantEntryCatalog)); + getAndAssertCountInEdit(filteredTenantEntryCatalog, true)); //now check that a WAL.Entry with cells from both a tenant and a non-tenant //catalog row only allow the tenant cells through @@ -179,7 +170,7 @@ public class SystemCatalogWALEntryFilterIT extends ParallelStatsDisabledIT { WAL.Entry tenantEntryChildLink = getEntry(systemChildLinkTableName, tenantGetChildLink); WAL.Entry nonTenantEntryChildLink = getEntry(systemChildLinkTableName, nonTenantGetChildLink); - int tenantRowCount = getAndAssertTenantCountInEdit(tenantEntryChildLink); + int tenantRowCount = getAndAssertCountInEdit(tenantEntryChildLink, true); Assert.assertTrue(tenantRowCount > 0); //verify that the tenant view WAL.Entry passes the filter and the non-tenant view does not @@ -190,13 +181,13 @@ public class SystemCatalogWALEntryFilterIT extends ParallelStatsDisabledIT { Assert.assertTrue(nonTenantEntryChildLink.getEdit().size() > 0); // All the cells will get removed by the filter since they do not belong to tenant Assert.assertTrue("Non tenant edits for system child link should not get filtered", - chainWALEntryFilter.filter(nonTenantEntryChildLink).getEdit().isEmpty()); + chainWALEntryFilter.filter(nonTenantEntryChildLink).getEdit().isEmpty()); WAL.Entry filteredTenantEntryChildLink = chainWALEntryFilter.filter(tenantEntryChildLink); Assert.assertNotNull("Tenant view was filtered when it shouldn't be!", - filteredTenantEntryChildLink); + filteredTenantEntryChildLink); Assert.assertEquals("Not all data for replicated for tenant", tenantRowCount, - getAndAssertTenantCountInEdit(filteredTenantEntryChildLink)); + getAndAssertCountInEdit(filteredTenantEntryChildLink, true)); //now check that a WAL.Entry with cells from both a tenant and a non-tenant // child link row only allow the tenant cells through @@ -206,12 +197,54 @@ public class SystemCatalogWALEntryFilterIT extends ParallelStatsDisabledIT { WAL.Entry comboEntry = new WAL.Entry(walKeyChildLink, comboEdit); Assert.assertEquals(tenantEntryChildLink.getEdit().size() + nonTenantEntryChildLink.getEdit().size() - , comboEntry.getEdit().size()); + , comboEntry.getEdit().size()); Assert.assertEquals(tenantEntryChildLink.getEdit().size(), - chainWALEntryFilter.filter(comboEntry).getEdit().size()); + chainWALEntryFilter.filter(comboEntry).getEdit().size()); + } + + /** + * Validates the behavior for parent-child link's delete marker via SystemCatalogWalEntryFilter. + * 1. Filtered for non-tenant views. + * 2. Not filtered for tenant views. + * */ + @Test + public void testDeleteMarkerForParentChildLink() throws Exception{ + // Since for 4.16+ all parent-child links are stored in SYSTEM.CHILD_LINK, only + // checking for that table in this test. + + // Make sure link row exists. + WAL.Entry childLinkEntry = getEntry(systemChildLinkTableName, new Scan(), + false); + int tenantRowCount = getAndAssertCountInEdit(childLinkEntry, true); + int nonTenantRowCount = getAndAssertCountInEdit(childLinkEntry, false); + Assert.assertTrue(tenantRowCount > 0 && nonTenantRowCount > 0 ); + + // Drop both tenant and non-tenant view. + dropTenantView(); + dropNonTenantView(); + + // Delete Marker for non-tenant view should get filtered and for tenant-view it should not. + SystemCatalogWALEntryFilter filter = new SystemCatalogWALEntryFilter(); + // Chain the system catalog WAL entry filter to ChainWALEntryFilter + ChainWALEntryFilter chainWALEntryFilter = new ChainWALEntryFilter(filter); + childLinkEntry = getEntry(systemChildLinkTableName, new Scan(), + false); + int tenantDeleteCountBeforeFilter = getDeleteFamilyCellCountInEntry(childLinkEntry, true); + int nonTenantDeleteCountBeforeFilter = getDeleteFamilyCellCountInEntry(childLinkEntry, false); + // Make sure both tenant and non-tenant delete marker exists before filtering + Assert.assertTrue(tenantDeleteCountBeforeFilter > 0 && nonTenantDeleteCountBeforeFilter > 0 ); + + WAL.Entry filteredEntry = chainWALEntryFilter.filter(childLinkEntry); + int tenantDeleteCountAfterFilter = getDeleteFamilyCellCountInEntry(filteredEntry, true); + int nonTenantDeleteCountAfterFilter = getDeleteFamilyCellCountInEntry(filteredEntry, false); + Assert.assertTrue(tenantDeleteCountAfterFilter == tenantDeleteCountBeforeFilter && nonTenantDeleteCountAfterFilter == 0 ); + + // setup views again. + createTenantView(); + createNonTenantView(); } - public Get getGet(PTable catalogTable, byte[] tenantId, String viewName) { + private Get getGet(PTable catalogTable, byte[] tenantId, String viewName) { byte[][] tenantKeyParts = new byte[5][]; tenantKeyParts[0] = tenantId; tenantKeyParts[1] = Bytes.toBytes(SCHEMA_NAME.toUpperCase()); @@ -225,7 +258,7 @@ public class SystemCatalogWALEntryFilterIT extends ParallelStatsDisabledIT { return new Get(key.copyBytes()); } - public Get getGetChildLink(PTable catalogTable, byte[] tenantId, String viewName) { + private Get getGetChildLink(PTable catalogTable, byte[] tenantId, String viewName) { byte[][] tenantKeyParts = new byte[5][]; tenantKeyParts[0] = ByteUtil.EMPTY_BYTE_ARRAY; tenantKeyParts[1] = ByteUtil.EMPTY_BYTE_ARRAY; @@ -246,21 +279,54 @@ public class SystemCatalogWALEntryFilterIT extends ParallelStatsDisabledIT { boolean isChildLinkForTenantId = row.contains(tenantId) && CellUtil.matchingQualifier(cell, PhoenixDatabaseMetaData.LINK_TYPE_BYTES); - return isTenantIdLeading || isChildLinkForTenantId; + boolean isDeleteMarkerForLinkRow = row.contains(tenantId) && CellUtil.isDeleteFamily(cell); + return isTenantIdLeading || isChildLinkForTenantId || isDeleteMarkerForLinkRow; } - private int getAndAssertTenantCountInEdit(WAL.Entry entry) { - int count = 0; + /** + * Asserts and returns cell count in the WAL.Entry. if tenantOwned is true, tenant owned cell count is + * returned else non-tenant cell count. + * @Param entry {@link WAL.Entry} + * @Param tenantOwned {@link Boolean} + * */ + private int getAndAssertCountInEdit(WAL.Entry entry, boolean tenantOwned) { + int tenantCount = 0; + int nonTenantCount = 0; for (Cell cell : entry.getEdit().getCells()) { if (isTenantOwnedCell(cell, TENANT_ID)) { - count = count + 1; + tenantCount = tenantCount + 1; + } else { + nonTenantCount = nonTenantCount + 1; } } + int count = tenantOwned ? tenantCount : nonTenantCount; Assert.assertTrue(count > 0); return count; } - public WAL.Entry getEntry(TableName tableName, Get get) throws IOException { + /** + * Returns delete family cell count in the WAL.Entry. if tenantOwned is true, tenant owned cell count is + * returned else non-tenant cell count. + * @Param entry {@link WAL.Entry} + * @Param tenantOwned {@link Boolean} + * */ + private int getDeleteFamilyCellCountInEntry(WAL.Entry entry, boolean tenantOwned) { + int tenantCount = 0; + int nonTenantCount = 0; + for (Cell cell : entry.getEdit().getCells()) { + if (CellUtil.isDeleteFamily(cell)) { + if (isTenantOwnedCell(cell, TENANT_ID)) { + tenantCount = tenantCount + 1; + } else { + nonTenantCount = nonTenantCount + 1; + } + } + } + return tenantOwned ? tenantCount : nonTenantCount; + } + + + private WAL.Entry getEntry(TableName tableName, Get get) throws IOException { WAL.Entry entry = null; try(Connection conn = ConnectionFactory.createConnection(getUtility().getConfiguration())){ Table htable = conn.getTable(tableName); @@ -281,4 +347,68 @@ public class SystemCatalogWALEntryFilterIT extends ParallelStatsDisabledIT { } return entry; } + + private WAL.Entry getEntry(TableName tableName, Scan scan, boolean addIndexedKeyValueCell) + throws IOException { + WAL.Entry entry = null; + try(HConnection conn = HConnectionManager.createConnection(getUtility().getConfiguration())) { + HTableInterface htable = conn.getTable(tableName); + scan.setRaw(true); + ResultScanner scanner = htable.getScanner(scan); + WALEdit edit = new WALEdit(); + if (addIndexedKeyValueCell) { + // add IndexedKeyValue type cell as the first cell + edit.add(new IndexedKeyValue()); + } + + for (Result r : scanner) { + if (r != null) { + List<Cell> cellList = r.listCells(); + for (Cell c : cellList) { + edit.add(c); + } + } + } + Assert.assertFalse("No WALEdits were loaded!", edit.isEmpty()); + HLogKey key = new HLogKey(REGION, tableName, 0, 0, uuid); + entry = new WAL.Entry(key, edit); + } + return entry; + } + + private static void dropTenantView() throws Exception { + Properties tenantProperties = new Properties(); + tenantProperties.setProperty("TenantId", TENANT_ID); + try (java.sql.Connection connection = + ConnectionUtil.getInputConnection(getUtility().getConfiguration(), tenantProperties)) { + connection.createStatement().execute(DROP_TENANT_VIEW_SQL); + connection.commit(); + } + } + + private static void dropNonTenantView() throws Exception { + try (java.sql.Connection connection = + ConnectionUtil.getInputConnection(getUtility().getConfiguration(), new Properties())) { + + connection.createStatement().execute(DROP_NONTENANT_VIEW_SQL); + } + } + + private static void createTenantView() throws Exception { + Properties tenantProperties = new Properties(); + tenantProperties.setProperty("TenantId", TENANT_ID); + try (java.sql.Connection connection = + ConnectionUtil.getInputConnection(getUtility().getConfiguration(), tenantProperties)) { + connection.createStatement().execute(CREATE_TENANT_VIEW_SQL); + connection.commit(); + } + } + + private static void createNonTenantView() throws Exception { + try (java.sql.Connection connection = + ConnectionUtil.getInputConnection(getUtility().getConfiguration(), new Properties())) { + connection.createStatement().execute(CREATE_NONTENANT_VIEW_SQL); + connection.commit(); + } + } } diff --git a/phoenix-core/src/main/java/org/apache/phoenix/replication/SystemCatalogWALEntryFilter.java b/phoenix-core/src/main/java/org/apache/phoenix/replication/SystemCatalogWALEntryFilter.java index 3f29e8e..874d810 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/replication/SystemCatalogWALEntryFilter.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/replication/SystemCatalogWALEntryFilter.java @@ -90,7 +90,8 @@ public class SystemCatalogWALEntryFilter implements * tenant id, system.child_link table have tenant owned data for parent child * links. In this case, the column qualifier is * {@code PhoenixDatabaseMetaData#LINK_TYPE_BYTES} and value is - * {@code PTable.LinkType.CHILD_TABLE}. + * {@code PTable.LinkType.CHILD_TABLE}. For corresponding delete markers the + * KeyValue type {@code KeyValue.Type} is {@code KeyValue.Type.DeleteFamily} * @param cell hbase cell * @return true if the cell is tenant owned */ @@ -103,18 +104,20 @@ public class SystemCatalogWALEntryFilter implements if (!isTenantRowCell) { boolean isChildLink = CellUtil.matchingQualifier( cell, PhoenixDatabaseMetaData.LINK_TYPE_BYTES); - if (isChildLink) { - if (CellUtil.matchingValue(cell, CHILD_TABLE_BYTES)) { + // Check if cell is of type LINK_TYPE with value 4 or DeleteFamily + if ((isChildLink && CellUtil.matchingValue(cell, CHILD_TABLE_BYTES)) || + CellUtil.isDeleteFamily(cell) ) { byte[][] rowViewKeyMetadata = new byte[NUM_COLUMNS_PRIMARY_KEY][]; SchemaUtil.getVarChars(key.get(), key.getOffset(), key.getLength(), 0, rowViewKeyMetadata); - // if the child link is to a tenant-owned view, - // the COLUMN_NAME field will be the byte[] of the tenant - //otherwise, it will be an empty byte array - // (NOT QueryConstants.SEPARATOR_BYTE, but a byte[0]) + /** if the child link is to a tenant-owned view, the COLUMN_NAME field will be + * the byte[] of the tenant otherwise, it will be an empty byte array + * (NOT QueryConstants.SEPARATOR_BYTE, but a byte[0]). This assumption is also + * true for child link's delete markers in SYSTEM.CHILD_LINK as it only contains link + * rows and does not deal with other type of rows like column rows that also has + * COLUMN_NAME populated with actual column name.**/ isChildLinkToTenantView = rowViewKeyMetadata[COLUMN_NAME_INDEX].length != 0; - } } } return isTenantRowCell || isChildLinkToTenantView;