Skip to content

PHOENIX-6437: Parent-Child Delete marker should get replicated via Sy…#1219

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gjacoby126 merged 2 commits into from
May 5, 2021
Merged

PHOENIX-6437: Parent-Child Delete marker should get replicated via Sy… #1219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2d8f672
PHOENIX-6437: Parent-Child Delete marker should get replicated via Sy…
May 3, 2021
d7e154f
Review Comments
May 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
194 changes: 162 additions & 32 deletions phoenix-core/src/it/java/org/apache/phoenix/replication/SystemCatalogWALEntryFilterIT.java
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -22,12 +22,14 @@
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;
Expand DownExpand Up@@ -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;
Expand DownExpand Up@@ -97,24 +99,13 @@ public static synchronized void setup() throws Exception {
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
Expand All@@ -136,7 +127,7 @@ public void testSystemCatalogWALEntryFilter() throws Exception {

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
Expand All@@ -151,9 +142,9 @@ public void testSystemCatalogWALEntryFilter() throws Exception {

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
Expand All@@ -179,7 +170,7 @@ public void testSystemChildLinkWALEntryFilter() throws Exception {

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
Expand All@@ -190,13 +181,13 @@ public void testSystemChildLinkWALEntryFilter() throws Exception {
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
Expand All@@ -206,12 +197,54 @@ public void testSystemChildLinkWALEntryFilter() throws Exception {
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());
Expand All@@ -225,7 +258,7 @@ public Get getGet(PTable catalogTable, byte[] tenantId, String viewName) {
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;
Expand All@@ -246,21 +279,54 @@ private boolean isTenantOwnedCell(Cell cell, String tenantId) {
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);
Expand All@@ -281,4 +347,68 @@ public WAL.Entry getEntry(TableName tableName, Get get) throws IOException {
}
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();
}
}
}
19 changes: 11 additions & 8 deletions phoenix-core/src/main/java/org/apache/phoenix/replication/SystemCatalogWALEntryFilter.java
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -90,7 +90,8 @@ private boolean doesKeyHaveLeadingSeparatorByte(final Cell cell) {
* 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
*/
Expand All@@ -103,18 +104,20 @@ private boolean isTenantRowCellSystemChildLink(final Cell cell) {
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) ) {

@xcangCRMxcangCRMMay 4, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this logic means you will replicate whenever it's a DeleteFamily cell, regardless of childlink or not. Is this the intention?

@ankitjain64ankitjain64May 4, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To replicate a delete marker we are using two checks.

  1. The cell is of type DeleteFamily.
  2. The length for COLUMN_NAME is greater than 0. (For tenant views column_name is populated with tenant_id which is not the case for global views.)

One may argue that the above two checks are also true for delete markers of column rows and we may over-replicate those rows. But, we are avoiding all those scenarios by doing the above filtering only for the mutations belonging to SYSTEM.CHILD_LINK table. With 4.15+ we are assured that SYSTEM.CHILD_LINK table only stores parent-child linking rows with LINK_TYPE=4, so the only thing we need to differentiate here is parent-child link delete markers for tenant vs non-tenant view. Let me know if you still feel we are missing any scenario. Thanks

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;
Expand Down