PHOENIX-4799 Write cells using checkAndMutate to prevent conflicting … #313

twdsilva · 2018-07-24T18:00:38Z

…changes

@vincentpoon @karanmehta93 @ChinmaySKulkarni Can you please review?
With this patch, when we add a column we write a cell to SYSTEM.CHILD_LINK with row key (tenantId, schemaName, physicalTableName, columnName) to prevent conflicting concurrent modifications.
While dropping a table or creating a view we also write a cell to SYSTEM.CHILD_LINK with row key (tenantId, schemaName, physicalTableName).
This is done in MetadataClient before we make an rpc to create view / drop table / add column.

karanmehta93 · 2018-07-24T18:15:07Z

Just curious, why not use SYSTEM.MUTEX table for this? Its a table specially added for use-cases that need time based expiration and distributed locks.

twdsilva · 2018-07-24T18:34:58Z

Thanks for the suggestion, I will update the patch to use SYSTEM.MUTEX

twdsilva · 2018-07-24T23:51:34Z

@karanmehta93 I have updated the PR to use SYSTEM.MUTEX, please review.

ChinmaySKulkarni · 2018-07-25T02:54:54Z

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

+
+			// add a new column to the base table to ensure that the cell used
+			// to prevent concurrent modifications was removed
+			tableDdl = "ALTER TABLE " + fullTableName + " ADD v4 INTEGER";


You didn't execute this ddl statement

ChinmaySKulkarni · 2018-07-25T02:55:05Z

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

+			latch2.countDown();
+
+			Exception e = future.get();
+			assertTrue(e == null);


nit: assertNull

ChinmaySKulkarni · 2018-07-25T02:56:12Z

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

+				// multiple times
+				throw new DoNotRetryIOException();
+			} else if (shouldSlowDown(c, miniBatchOp.getOperation(0))) {
+				// simulate a slow write to SYSTEM.CATALOG


ChinmaySKulkarni · 2018-07-25T02:57:32Z

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

+	}
+
+	/**
+	 * Remove the cell that was written to to the SYSTEM.CHILD_LINK table with


Update the comment since you're using SYSMUTEX now

ChinmaySKulkarni · 2018-07-25T02:58:26Z

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

@@ -3034,6 +3086,10 @@ MutationState dropTable(String schemaName, String tableName, String parentTableN
            return new MutationState(0, 0, connection);
        } finally {
            connection.setAutoCommit(wasAutoCommit);
+            // lock the table to prevent concurrent table modifications


Update comment

ChinmaySKulkarni · 2018-07-25T02:59:33Z

A few comments and nits. Do we have to do anything special to handle indexes on views? I guess a view on a view should be fine with this implementation since the physical table name would resolve to the final base table. @twdsilva

twdsilva · 2018-07-26T00:17:04Z

We can't add/drop columns directly to an index on a view. Views with their own child views will write a mutex using the physical table.

@ChinmaySKulkarni and @karanmehta93 thanks for the feedback. I have updated the PR, please take a look.

ankitsinghal

@twdsilva , general comment
how we are protecting such mutation in case we are using the old client?

ankitsinghal · 2018-07-26T20:38:37Z

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

+                            SchemaUtil.getSchemaNameFromFullName(physicalName.getString());
+                    String physicalTableName =
+                            SchemaUtil.getTableNameFromFullName(physicalName.getString());
+                    deleteCell(null, physicalSchemaName, physicalTableName,


@twdsilva , shouldn't you check whether you have acquired a lock(or inserted the cell in Mutex) before deleting the lock cell, because here you might be deleting the locks acquired by some other threads. And, You may need to do this per column I think.

I changed the code to only delete the cells if we were able to successfully do the checkAndPut.
When we add a column we write a cell per column that we are creating.
When we drop a base table or create a view, we write a single cell with the rowkey of the physical table (to prevent a view being created while we are dropping the base table).

ankitsinghal · 2018-07-26T20:39:12Z

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

+            // dropping the base table
+            if (tableType == PTableType.TABLE) {
+                deleteCell(null, schemaName, tableName, null);
+            }


nit: Please update the comment here that you are releasing a mutex.

twdsilva · 2018-07-27T22:46:39Z

@ankitsinghal I filed PHOENIX-4765 to not allow metadata changes on a base table that has child views when the request is from an older client. This will also allow us to rollback the upgraded server side jar if required.

twdsilva · 2018-07-28T18:19:04Z

@ankitsinghal Thanks for the review. I have updated the PR to based on the feedback. Can you please take a look?

ankitsinghal · 2018-07-30T21:37:10Z

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

@@ -3604,6 +3681,18 @@ public MutationState addColumn(PTable table, List<ColumnDef> origColumnDefs,
            }
        } finally {
            connection.setAutoCommit(wasAutoCommit);
+            if (acquiredMutex && !columns.isEmpty()) {


where is acquiredMutex set to true?

@ankitsinghal Sorry, I didn't update the PR with all my latest changes, I fixed this. Can you please review?

karanmehta93 · 2018-07-31T06:56:22Z

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

+    static final String SYSTEM_MUTEX_IDENTIFIER =
+            QueryConstants.SYSTEM_SCHEMA_NAME + "." + "\""
+                    + PhoenixDatabaseMetaData.SYSTEM_MUTEX_TABLE_NAME + "\"";
+
    static final Set<String> PHOENIX_NAMESPACE_MAPPED_SYSTEM_TABLES = new HashSet<>(Arrays.asList(


Do you need to add SYSTEM:MUTEX over here as well since now its visible from Phoenix level as well?

karanmehta93 · 2018-07-31T07:04:16Z

phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java

@@ -2723,6 +2712,9 @@ private void createOtherSystemTables(PhoenixConnection metaConnection, HBaseAdmi
        try {
            metaConnection.createStatement().executeUpdate(getChildLinkDDL());
        } catch (TableAlreadyExistsException e) {}
+        try {
+            metaConnection.createStatement().executeUpdate(getMutexDDL());
+        } catch (TableAlreadyExistsException e) {}
        // Catch the IOException to log the error message and then bubble it up for the client to retry.
        try {
            createSysMutexTableIfNotExists(hbaseAdmin);


Do we still need this part of code?

karanmehta93 · 2018-07-31T07:05:33Z

phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java

@@ -3410,17 +3405,10 @@ void ensureSystemTablesMigratedToSystemNamespace()
            // No tables exist matching "SYSTEM\..*", they are all already in "SYSTEM:.*"
            if (tableNames.size() == 0) { return; }
            // Try to move any remaining tables matching "SYSTEM\..*" into "SYSTEM:"
-            if (tableNames.size() > 5) {
+            if (tableNames.size() > 7) {
                logger.warn("Expected 5 system tables but found " + tableNames.size() + ":" + tableNames);


nit: change log message appropriately. Extract the constant to a variable

karanmehta93 · 2018-07-31T07:09:44Z

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

@@ -1913,6 +1940,21 @@ private PTable createTableInternal(CreateTableStatement statement, byte[][] spli
            boolean isLocalIndex = indexType == IndexType.LOCAL;
            QualifierEncodingScheme encodingScheme = NON_ENCODED_QUALIFIERS;
            ImmutableStorageScheme immutableStorageScheme = ONE_CELL_PER_COLUMN;
+
+            if (tableType == PTableType.VIEW) {


nit: Would be good to have some logs here for debugging purposes.

karanmehta93 · 2018-07-31T07:11:24Z

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

+            if (tableType == PTableType.TABLE) {
+                // acquire a mutex on the table to prevent creating views while concurrently
+                // dropping the base table
+                acquiredMutex = writeCell(null, schemaName, tableName, null);


Nothing is prevented even if acquiredMutex is false here. I think you should throw ConcurrentTableMutationException here as well? You might might wanna push down that logic to writeCell() method?

Yes, I missed that. I originally had writeCell throw the ConcurrentTableMutationException, but we need to know we need writeCell to return false if it wasn't able to do the checkAndPut so that we can set the acquiredMutex boolean which is later used to determine if we have to delete the cell in the finally block.

karanmehta93 · 2018-07-31T07:15:18Z

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

@@ -16,8 +16,29 @@
 */
 package org.apache.phoenix.end2end;

-import com.google.common.base.Joiner;
-import com.google.common.base.Throwables;
+import static org.junit.Assert.assertEquals;


nit: Diff generated due to change in order of imports

I used the dev/phoenix.importorder format, so this should be fine.

twdsilva · 2018-07-31T21:23:07Z

@karanmehta93 or @ankitsinghal Can you please review the updated PR, thanks!

ankitsinghal

Thanks @twdsilva for taking care of review comments, have left some more review comments on the recent changes.

ankitsinghal · 2018-08-01T02:09:04Z

phoenix-core/src/main/java/org/apache/phoenix/query/QueryConstants.java

+	            + TABLE_SCHEM + "," + TABLE_NAME + "," + COLUMN_NAME + "," + COLUMN_FAMILY + "))\n" +
+	            HConstants.VERSIONS + "=%s,\n" +
+	            HColumnDescriptor.KEEP_DELETED_CELLS + "=%s,\n" +
+	            PhoenixDatabaseMetaData.TRANSACTIONAL + "=" + Boolean.FALSE;


Why there is a need of creating a Phoenix managed table for mutex?
And also API in QueryServices.writeMutexCell(byte[] rowKey) and deleteMutexCell(byte[] rowKey) don't enforce the schema of the table will be followed.

For clusters that use namespace mapping and that map the phoenix system tables to the SYSTEM namespace, we want to be able to use the GRANT/REVOKE statements to grant RW access to the SYSTEM:MUTEX table.

Are you suggesting writeMutexCell/deleteMutexCell should take as arguments (tenantid, schema, tablename, column name, column family) instead of a byte[]? I will make that change.

ok make sense, so can you just update QueryServices.writeMutexCell(byte[] rowKey) and deleteMutexCell(byte[] rowKey) to accept all the arguments which form the right primary key for the table just for consistency.

Yes, probably our comments have crossed.

ankitsinghal · 2018-08-01T02:11:23Z

phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java

-            logger.error("Failed to created SYSMUTEX table. Upgrade or migration is not possible without it. Please retry.");
-            throw exception;
-        }
+            metaConnection.createStatement().executeUpdate(getMutexDDL());


Shouldn't the mutex table available and have acquired a mutex already for the upgrade before you call createOtherSystemTables

The mutex table would have been created by createSysMutexTableIfNotExists(), we call execute the CREATE TABLE statement so that it exists in SYSTEM.CATALOG, so that we can use GRANT/REVOKE to grant permission on the SYSTEM.MUTEX table.

ok, then we are good.

twdsilva · 2018-08-02T00:33:25Z

Testing issues@phoenix.apache.org.

twdsilva · 2018-08-03T00:08:50Z

@ankitsinghal @karanmehta93 I updated the PR with the latest feedback, can you please review?

karanmehta93 · 2018-08-03T21:44:12Z

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

+                // dropping the base table
+                acquiredMutex = writeCell(null, schemaName, tableName, null);
+                if (!acquiredMutex) {
+                    logger.info("Failed to acquire mutex on physical table " + physicalTableName);


nit: You can push mutex related logs lines to the specific methods.

Done, can I get a +1?

…changes

twdsilva force-pushed the PHOENIX-4799 branch 3 times, most recently from e7dbe0e to fb916c4 Compare July 24, 2018 23:50

twdsilva force-pushed the PHOENIX-4799 branch 2 times, most recently from f4bbaf9 to 39f39e1 Compare July 25, 2018 01:50

ChinmaySKulkarni reviewed Jul 25, 2018

View reviewed changes

twdsilva force-pushed the PHOENIX-4799 branch from 39f39e1 to ba3d963 Compare July 25, 2018 18:03

asfgit force-pushed the master branch from 6b363b3 to 8a874cc Compare July 25, 2018 22:35

twdsilva force-pushed the PHOENIX-4799 branch 2 times, most recently from a16a14f to 65cc9b1 Compare July 25, 2018 23:49

twdsilva changed the base branch from master to 4.x-HBase-1.3 July 26, 2018 00:07

twdsilva force-pushed the PHOENIX-4799 branch from 65cc9b1 to 66e5fef Compare July 26, 2018 00:15

twdsilva force-pushed the PHOENIX-4799 branch from 66e5fef to cb67447 Compare July 26, 2018 17:57

ankitsinghal reviewed Jul 26, 2018

View reviewed changes

twdsilva force-pushed the PHOENIX-4799 branch from cb67447 to f31c98e Compare July 27, 2018 21:41

twdsilva force-pushed the PHOENIX-4799 branch from f31c98e to 4771541 Compare July 28, 2018 18:17

ankitsinghal reviewed Jul 30, 2018

View reviewed changes

twdsilva force-pushed the PHOENIX-4799 branch 2 times, most recently from b49a625 to 7412c39 Compare July 31, 2018 06:51

karanmehta93 reviewed Jul 31, 2018

View reviewed changes

twdsilva force-pushed the PHOENIX-4799 branch from 7412c39 to 3cf92e3 Compare July 31, 2018 21:15

ankitsinghal requested changes Aug 1, 2018

View reviewed changes

twdsilva force-pushed the PHOENIX-4799 branch from 3cf92e3 to 7a97940 Compare August 3, 2018 00:07

twdsilva force-pushed the PHOENIX-4799 branch 2 times, most recently from 16db8aa to 8a93ada Compare August 3, 2018 17:57

karanmehta93 reviewed Aug 3, 2018

View reviewed changes

twdsilva force-pushed the PHOENIX-4799 branch from 8a93ada to b3d180b Compare August 3, 2018 22:25

PHOENIX-4799 Write cells using checkAndMutate to prevent conflicting …

581b9b8

…changes

twdsilva force-pushed the PHOENIX-4799 branch from b3d180b to 581b9b8 Compare August 3, 2018 22:36

karanmehta93 approved these changes Aug 3, 2018

View reviewed changes

twdsilva closed this Aug 25, 2018

PHOENIX-4799 Write cells using checkAndMutate to prevent conflicting … #313

PHOENIX-4799 Write cells using checkAndMutate to prevent conflicting … #313

Conversation

twdsilva commented Jul 24, 2018

karanmehta93 commented Jul 24, 2018

twdsilva commented Jul 24, 2018

twdsilva commented Jul 24, 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

ChinmaySKulkarni commented Jul 25, 2018

twdsilva commented Jul 26, 2018

ankitsinghal left a 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 27, 2018

twdsilva commented Jul 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karanmehta93 Jul 31, 2018 • edited Loading

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

karanmehta93 Jul 31, 2018 • edited Loading

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 31, 2018

ankitsinghal left a comment • edited Loading

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

ankitsinghal Aug 1, 2018 • edited Loading

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 Aug 2, 2018

twdsilva commented Aug 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karanmehta93 Jul 31, 2018 •

edited

Loading

karanmehta93 Jul 31, 2018 •

edited

Loading

ankitsinghal left a comment •

edited

Loading

ankitsinghal Aug 1, 2018 •

edited

Loading