PHOENIX-3534 Support multi region SYSTEM.CATALOG table #303

twdsilva · 2018-05-29T17:08:46Z

This patch adds two new LinkTypes EXCLUDED_COLUMNS (used to represent a column that has been dropped) and VIEW_INDEX_PARENT_TABLE (used to link an index on a view to its parent). Views and view indexes no longer store columns derived from their ancestors in their metadata. When they are resolved the ancestors are looked up and added to the PTable that is returned to the client (see combineColumns in MetadataEndpointImpl). The PTable in the server side metadata cache only stores the columns created in the view/view index and not derived columns.
We do not propagate metadata changes made to a parent to all its children. While adding columns to a base table, we no longer lock all the children in the view hierarchy, we only validate that the columns being added does not conflict with an existing base table column. We also don't lock children while dropping a parent table column. When dropping a parent column we also drop any view indexes that need the column. This patch does not handle the case when there are concurrent changes (eg. adding a conflicting column or creating a new view index that requires a parent column that is being dropped). That will be handled in a follow-up patch.
While dropping a parent table, we don't drop all the child views metadata. This metadata needs to be cleaned-up (maybe at compaction time?) which will be handled in a follow-up patch.

There are a few test failures I am working through, which I will fix soon and update the PR.
@JamesRTaylor can you please review?

FYI @karanmehta93 @ChinmaySKulkarni

…nderResult

… additive case where we take lower timestamp

…ombine logic for excluded columns

…t always seem to work

Conflicts: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java phoenix-core/src/main/java/org/apache/phoenix/coprocessor/WhereConstantParser.java phoenix-core/src/test/java/org/apache/phoenix/coprocessor/MetaDataEndpointImplTest.java

…IX-3534

gjacoby126 · 2018-06-25T14:40:16Z

phoenix-core/src/main/java/org/apache/phoenix/replication/SystemCatalogWALEntryFilter.java

    //TODO: when Phoenix drops support for pre-1.3 versions of HBase, redo as a WALCellFilter
-    if (!SchemaUtil.isMetaTable(entry.getKey().getTablename().getName())){
+    byte[] tableName = entry.getKey().getTablename().getName();
+	if (!SchemaUtil.isMetaTable(tableName) && !SchemaUtil.isChildLinkTable(tableName)){


Would it be safe to turn on normal HBase replication on the new System.CHILD_LINK? (That is, is there any unwanted data in System.CHILD_LINK that this WALFilter wouldn't copy that normal HBase replication would?)

If normal HBase replication works for System.CHILD_LINK, and all view data left in System.Catalog starts with tenant_id, then the logic here can be greatly simplified, similar to how it was before PHOENIX-4229

SYSTEM.CHILD_LINK contains the parent->child linking rows and cells we use to detect race conditions (eg a column of conflicting type being added at the same time to a parent and child).
The latter cells are written with a short TTL.
I think we can use HBase replication for SYSTEM.CHILD_LINK. All the tenant specific view metadata rows in SYSTEM.CATALOG start with tenant id.
I will modify this filter to how it was before PHOENIX-4229.
@gjacoby126 Thanks for the suggestion.

…eak tenant view replication" This reverts commit ff80555.

…n't been cleaned up yet)

…lit finishes

…, adding a column

JamesRTaylor · 2018-07-04T23:19:04Z

+1 to the patch. Great work @twdsilva and @Churrodog! I made some minor comments for some potential follow up work and had a few questions, but let's get this committed first. I'd recommend the following priority for the next JIRA as:

Move views to their own table
Get rid of client side code that is sending the base columns
Fix corner case/race condition issues
Add code that doesn't write orphaned metadata on major compaction

JamesRTaylor · 2018-07-04T22:10:32Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java

+            PColumn pColumn = myColumns.get(i);
+            if (pColumn.isExcluded()) {
+                excludedColumns.add(pColumn);
+            } else if (!pColumn.equals(SaltingUtil.SALTING_COLUMN)) { 


Instead of matching on SALTING_COLUMN, we should stop the loop at 1 above if table.getSaltBuckets() != null. The code never filters the salt column based on it's name.

I will change this.

JamesRTaylor · 2018-07-04T22:16:48Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java

+                    // add all pk columns of parent tables to indexes
+                    for (PColumn column : pTable.getPKColumns()) {
+                        // don't add the salt column of ancestor tables for view indexes
+                        if (column.equals(SaltingUtil.SALTING_COLUMN) || column.isExcluded()) {


Same comment as before - we should be able to match based on the column being the first column. Note that the salt column would only be in the base physical table.

I will change this.

JamesRTaylor · 2018-07-04T22:20:16Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java

        try {
            int clientVersion = request.getClientVersion();
            List<Mutation> tableMetadata = ProtobufUtil.getMutations(request);
            MetaDataUtil.getTenantIdAndSchemaAndTableName(tableMetadata, rowKeyMetaData);
            byte[] tenantIdBytes = rowKeyMetaData[PhoenixDatabaseMetaData.TENANT_ID_INDEX];
            schemaName = rowKeyMetaData[PhoenixDatabaseMetaData.SCHEMA_NAME_INDEX];
            tableName = rowKeyMetaData[PhoenixDatabaseMetaData.TABLE_NAME_INDEX];
+            fullTableName = SchemaUtil.getTableName(schemaName, tableName);
+            // TODO before creating a table we need to see if the table was previously created and then dropped
+            // and clean up any parent->child links or child views


Remove TODO as isn't this done now?

JamesRTaylor · 2018-07-04T22:21:10Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java

+			// been cleaned up by compaction
+			if (!Bytes.toString(schemaName).equals(QueryConstants.SYSTEM_SCHEMA_NAME)) {
+				dropChildMetadata(schemaName, tableName, tenantIdBytes);
+			}


Minor - indentation issue here.

JamesRTaylor · 2018-07-04T22:22:00Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java

+				/*
+				 * For a mapped view, there is no link present to the physical table. So the
+				 * viewPhysicalTableRow is null in that case.
+				 */


Fix indentation

JamesRTaylor · 2018-07-04T22:51:17Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java

@@ -447,7 +447,7 @@
    static {
        Collections.sort(FUNCTION_KV_COLUMNS, KeyValue.COMPARATOR);
    }
-    
+


Might be good to include a class level comment that explains the overall approach at a high level.

I modified the class level comment.

JamesRTaylor · 2018-07-04T22:54:18Z

phoenix-core/src/main/java/org/apache/phoenix/schema/TableProperty.java

-        this(propertyName, colFamSpecifiedException, isMutable, mutatingException, isValidOnView, isMutableOnView, true);
-    }
-
-    private TableProperty(String propertyName, SQLExceptionCode colFamSpecifiedException, boolean isMutable, SQLExceptionCode mutatingException, boolean isValidOnView, boolean isMutableOnView, boolean propagateToViews) {


How did you end up dealing with table property conflicts between parent and children? Is there follow up work required? Can we use the timestamp of the Cell storing the property to differentiate similar to the logic for columns? It's fine to do this work in a follow up JIRA.

I filed PHOENIX-4763 to fix this. We should be able to use the cell timestamp to differentiate, still need to figure out how to expose this since its the properties in PTable don't currently expose the timestamp.

JamesRTaylor · 2018-07-04T22:57:51Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java

+                // 3. Finally write the mutations to create the table
+
+                // From 4.15 the parent->child links are stored in a separate table SYSTEM.CHILD_LINK
+                List<Mutation> childLinkMutations = MetaDataUtil.removeChildLinks(tableMetadata);


TODO to remove this code in 4.16.

I filed PHOENIX-4810 and added a comment to reference this jira.

JamesRTaylor · 2018-07-04T23:00:07Z

phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java

-
+
+                // if the view is a first level child, then we need to create the PARENT_TABLE link
+                // that was overwritten by the PHYSICAL_TABLE link 


Ah, good. So we'll be consistent with the parent link now, right?

Yes this will make it so that the parent link row will be created correctly when upgrading tables to be namespace mapped.

JamesRTaylor · 2018-07-04T23:13:26Z

phoenix-core/src/it/java/org/apache/phoenix/end2end/ViewIT.java

+    }
+
+    @Test
+    public void testRecreateDroppedTableWithChildViews() throws Exception {


These new tests are good. These are testing that the left over metadata doesn't impact the re-creation of a table since we don't make the RPC to delete views when a base table is dropped, right? Do you think there'd be any issues if part of the rows for a view were there (i.e. say that the create view failed, but some of the rows were written)? Might be good to have a test like this - you could set it up by using HBase APIs to manually delete some rows of a view.

We write the parent->child link first, then if the table uses column encoding we update the encoded column qualifiers on the parent table, and finally use mutateRowsWithLocks to write the view metadata atomically.
We ignore views that can't be found (in case writing the child view metadata fails).
If the metadata write fails and the table uses column encoding then we will lose a few column qualifiers.
I'll add a test for this.

twdsilva · 2018-07-12T18:08:42Z

@JamesRTaylor Thanks for the feedback, I have updated the PR. I will get this committed shortly.

rgidwani and others added 30 commits March 17, 2017 16:14

Starting work on splittable System.Catalog

a7712e3

Removed all references to multi-region System.Catalog for TableViewFi…

220849f

…nderResult

Create table work, still trying to get rid of all columns

356cd43

Create table and read views work now

e01adb5

Fixed the test - moving on to add drop columns

19c7ce5

getting tests and add column to work

ab20f8d

Figuring out the delete logic and refactoring the old tests

ec35744

Added proto timestamp and exluded values to pcolumn also took care of…

7d41330

… additive case where we take lower timestamp

Drop Column Work in Progress, need to figure out how to resolve the c…

adfc5ce

…ombine logic for excluded columns

Alter view drop column works!

24414bd

Drop Cascade and create check completed, need to test

13b6e52

Drop cascade seems to work

0313cee

Phoenix 3534" to "PHOENIX-3534 Support multi region SYSTEM.CATALOG table

590689f

Fixing up a few things, resolving read columns for child views doesn'…

96c0570

…t always seem to work

Adding in the child view constants

ca64a0b

Adding in the child view constants

ac59c72

test fixes

c2addd4

Merge branch 'master' of https://github.com/apache/phoenix into PHOEN…

9127075

…IX-3534

Remove unnessarch txn manager in ConnectionlessQueryServicesImpl

a4b64f0

Merge branch 'master' into PHOENIX-3534

f1ea662

Rename link to VIEW_INDEX_PARENT

571e88b

Merge remote-tracking branch 'upstream/master' into PHOENIX-3534

308d2a8

fixed tests

f0e9670

add missing files

53fd995

add view index to parent links during upgrade

dcddc06

fix tests

508aa4b

fix test failures

c32dfbb

Merge remote-tracking branch 'upstream/master' into PHOENIX-3534

abd98d9

minor

7c32d46

twdsilva added 10 commits June 7, 2018 16:05

test fixes

93d993d

fix test failures

7af8823

Add missing files

a394498

test fix

46774c7

fix test failure

ae71ce1

Added ParentTalbeNotFoundExpcetion

a8aa523

Fix bug

149143c

removed loadTable() and getTableOnCurrentRegion()

fa4b7a7

Merge branch 'master' into PHOENIX-3534

932c281

m

a64e8e7

gjacoby126 reviewed Jun 25, 2018

View reviewed changes

twdsilva added 12 commits June 25, 2018 20:16

fix test failure

7ef3492

Revert "PHOENIX-4229 - Parent-Child linking rows in System.Catalog br…

580ad37

…eak tenant view replication" This reverts commit ff80555.

Allow recreating a table that was dropped (but the child metadata has…

e82104b

…n't been cleaned up yet)

Handle recreating dropped indexes and views

c6e9166

test fix

0773ece

Add config that determines if SYSTEM.CATALOG can split

e78e6a0

fix SystemTablePermissionsIT test failure

45b9317

Modify BaseTest to disable/enable instead of polling to see when a sp…

b6f8a73

…lit finishes

Add some comments about the order of mutations while creating a table…

6600deb

…, adding a column

Fix formatting

46fe70a

fix formatting

e9a6117

Modify comments

aec34de

JamesRTaylor reviewed Jul 4, 2018

View reviewed changes

twdsilva added 2 commits July 11, 2018 16:17

Add more comments, add test to test failure writing view metadata

fa9c4b9

Merge remote-tracking branch 'upstream/master' into PHOENIX-3534

dd9f537

Merge remote-tracking branch 'upstream/master' into PHOENIX-3534

02fba5b

twdsilva closed this Jul 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHOENIX-3534 Support multi region SYSTEM.CATALOG table #303

PHOENIX-3534 Support multi region SYSTEM.CATALOG table #303

twdsilva commented May 29, 2018

gjacoby126 Jun 25, 2018

twdsilva Jun 25, 2018

JamesRTaylor commented Jul 4, 2018

JamesRTaylor Jul 4, 2018

twdsilva Jul 11, 2018

JamesRTaylor Jul 4, 2018

twdsilva Jul 11, 2018

JamesRTaylor Jul 4, 2018

twdsilva Jul 11, 2018

JamesRTaylor Jul 4, 2018

twdsilva Jul 11, 2018

JamesRTaylor Jul 4, 2018

twdsilva Jul 11, 2018

JamesRTaylor Jul 4, 2018

twdsilva Jul 11, 2018

JamesRTaylor Jul 4, 2018

twdsilva Jul 14, 2018

JamesRTaylor Jul 4, 2018

twdsilva Jul 11, 2018

JamesRTaylor Jul 4, 2018

twdsilva Jul 11, 2018

JamesRTaylor Jul 4, 2018

twdsilva Jul 11, 2018

twdsilva commented Jul 12, 2018



		// if the view is a first level child, then we need to create the PARENT_TABLE link
		// that was overwritten by the PHYSICAL_TABLE link

PHOENIX-3534 Support multi region SYSTEM.CATALOG table #303

PHOENIX-3534 Support multi region SYSTEM.CATALOG table #303

Conversation

twdsilva commented May 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesRTaylor commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twdsilva commented Jul 12, 2018