[GitHub] [phoenix] gjacoby126 commented on a diff in pull request #1413: PHOENIX-6681 Optionally disable indexes during creation

[GitBox <gi...@apache.org>]

gjacoby126 commented on code in PR #1413:
URL: https://github.com/apache/phoenix/pull/1413#discussion_r847568894


##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexTwoPhaseCreateIT.java:
##########
@@ -0,0 +1,208 @@
+package org.apache.phoenix.end2end.index;
+
+import org.apache.hadoop.hbase.regionserver.ScanInfoUtil;
+import org.apache.phoenix.end2end.IndexToolIT;
+import org.apache.phoenix.end2end.transform.TransformToolIT;
+import org.apache.phoenix.jdbc.PhoenixConnection;
+import org.apache.phoenix.query.BaseTest;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.schema.PIndexState;
+import org.apache.phoenix.schema.PTable;
+import org.apache.phoenix.schema.PTableType;
+import org.apache.phoenix.schema.transform.SystemTransformRecord;
+import org.apache.phoenix.schema.transform.Transform;
+import org.apache.phoenix.thirdparty.com.google.common.collect.Maps;
+import org.apache.phoenix.util.PropertiesUtil;
+import org.apache.phoenix.util.ReadOnlyProps;
+import org.apache.phoenix.util.SchemaUtil;
+import org.apache.phoenix.util.StringUtil;
+import org.apache.phoenix.util.TestUtil;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Properties;
+
+import static org.apache.phoenix.query.QueryServices.INDEX_CREATE_DEFAULT_STATE;
+import static org.apache.phoenix.schema.PTable.ImmutableStorageScheme.ONE_CELL_PER_COLUMN;
+import static org.apache.phoenix.schema.PTable.QualifierEncodingScheme.NON_ENCODED_QUALIFIERS;
+import static org.apache.phoenix.util.TestUtil.INDEX_DATA_SCHEMA;
+import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
+import static org.apache.phoenix.util.TestUtil.getRowCount;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+public class IndexTwoPhaseCreateIT extends BaseTest {
+    @BeforeClass
+    public static synchronized void doSetup() throws Exception {
+        Map<String, String> props = Maps.newHashMapWithExpectedSize(1);
+        props.put(ScanInfoUtil.PHOENIX_MAX_LOOKBACK_AGE_CONF_KEY, Integer.toString(60 * 60)); // An hour
+        props.put(QueryServices.USE_STATS_FOR_PARALLELIZATION, Boolean.toString(false));
+        props.put(QueryServices.INDEX_CREATE_DEFAULT_STATE, PIndexState.CREATE_DISABLE.toString());
+        props.put(QueryServices.DEFAULT_IMMUTABLE_STORAGE_SCHEME_ATTRIB, "ONE_CELL_PER_COLUMN");
+        props.put(QueryServices.DEFAULT_COLUMN_ENCODED_BYTES_ATRRIB, "0");
+
+        setUpTestDriver(new ReadOnlyProps(props.entrySet().iterator()));
+    }
+
+    @AfterClass
+    public static synchronized void freeResources() throws Exception {
+        BaseTest.freeResourcesIfBeyondThreshold();
+    }
+
+    @Test
+    public void testIndexCreateWithNonDefaultSettings() throws Exception {
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+        String dataTable = generateUniqueName();
+        String indexName = "IDX_" + generateUniqueName();
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            String tableDDL = "CREATE TABLE " + dataTable + TestUtil.TEST_TABLE_SCHEMA;
+            conn.createStatement().execute(tableDDL);
+            BaseTest.upsertRows(conn, dataTable, 2);
+            String ddl = "CREATE INDEX " + indexName + " ON " + dataTable
+                    + " (varchar_col1 ASC, varchar_col2 ASC, int_pk DESC)"
+                    + " INCLUDE (int_col1, int_col2) ";
+            conn.createStatement().execute(ddl);
+
+            ResultSet rs = conn.getMetaData().getTables(null, null, indexName

Review Comment:
   nit: this will be more efficient if you pass in the empty string for the catalog (which will put a "TENANT_ID IS NULL" on the query)



##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexTwoPhaseCreateIT.java:
##########
@@ -0,0 +1,208 @@
+package org.apache.phoenix.end2end.index;
+
+import org.apache.hadoop.hbase.regionserver.ScanInfoUtil;
+import org.apache.phoenix.end2end.IndexToolIT;
+import org.apache.phoenix.end2end.transform.TransformToolIT;
+import org.apache.phoenix.jdbc.PhoenixConnection;
+import org.apache.phoenix.query.BaseTest;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.schema.PIndexState;
+import org.apache.phoenix.schema.PTable;
+import org.apache.phoenix.schema.PTableType;
+import org.apache.phoenix.schema.transform.SystemTransformRecord;
+import org.apache.phoenix.schema.transform.Transform;
+import org.apache.phoenix.thirdparty.com.google.common.collect.Maps;
+import org.apache.phoenix.util.PropertiesUtil;
+import org.apache.phoenix.util.ReadOnlyProps;
+import org.apache.phoenix.util.SchemaUtil;
+import org.apache.phoenix.util.StringUtil;
+import org.apache.phoenix.util.TestUtil;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Properties;
+
+import static org.apache.phoenix.query.QueryServices.INDEX_CREATE_DEFAULT_STATE;
+import static org.apache.phoenix.schema.PTable.ImmutableStorageScheme.ONE_CELL_PER_COLUMN;
+import static org.apache.phoenix.schema.PTable.QualifierEncodingScheme.NON_ENCODED_QUALIFIERS;
+import static org.apache.phoenix.util.TestUtil.INDEX_DATA_SCHEMA;
+import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
+import static org.apache.phoenix.util.TestUtil.getRowCount;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+public class IndexTwoPhaseCreateIT extends BaseTest {
+    @BeforeClass
+    public static synchronized void doSetup() throws Exception {
+        Map<String, String> props = Maps.newHashMapWithExpectedSize(1);
+        props.put(ScanInfoUtil.PHOENIX_MAX_LOOKBACK_AGE_CONF_KEY, Integer.toString(60 * 60)); // An hour
+        props.put(QueryServices.USE_STATS_FOR_PARALLELIZATION, Boolean.toString(false));
+        props.put(QueryServices.INDEX_CREATE_DEFAULT_STATE, PIndexState.CREATE_DISABLE.toString());
+        props.put(QueryServices.DEFAULT_IMMUTABLE_STORAGE_SCHEME_ATTRIB, "ONE_CELL_PER_COLUMN");
+        props.put(QueryServices.DEFAULT_COLUMN_ENCODED_BYTES_ATRRIB, "0");
+
+        setUpTestDriver(new ReadOnlyProps(props.entrySet().iterator()));
+    }
+
+    @AfterClass
+    public static synchronized void freeResources() throws Exception {
+        BaseTest.freeResourcesIfBeyondThreshold();
+    }
+
+    @Test
+    public void testIndexCreateWithNonDefaultSettings() throws Exception {
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+        String dataTable = generateUniqueName();
+        String indexName = "IDX_" + generateUniqueName();
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            String tableDDL = "CREATE TABLE " + dataTable + TestUtil.TEST_TABLE_SCHEMA;
+            conn.createStatement().execute(tableDDL);
+            BaseTest.upsertRows(conn, dataTable, 2);
+            String ddl = "CREATE INDEX " + indexName + " ON " + dataTable
+                    + " (varchar_col1 ASC, varchar_col2 ASC, int_pk DESC)"
+                    + " INCLUDE (int_col1, int_col2) ";
+            conn.createStatement().execute(ddl);
+
+            ResultSet rs = conn.getMetaData().getTables(null, null, indexName
+                    , new String[]{PTableType.INDEX.toString()});
+            assertTrue(rs.next());
+            assertEquals(indexName, rs.getString(3));
+            assertEquals(PIndexState.CREATE_DISABLE.toString(), rs.getString("INDEX_STATE"));
+            assertFalse(rs.next());
+
+            assertEquals(0, getRowCount(conn, indexName));
+
+            ddl = "ALTER INDEX " + indexName + " ON " + dataTable + " REBUILD ASYNC";
+            conn.createStatement().execute(ddl);
+            rs = conn.getMetaData().getTables(null, null, indexName
+                    , new String[]{PTableType.INDEX.toString()});
+            assertTrue(rs.next());
+            assertEquals(PIndexState.BUILDING.toString(), rs.getString("INDEX_STATE"));
+            assertFalse(rs.next());
+        }
+    }
+
+
+    @Test
+    public void testIndexCreateDisabledBuildAfter() throws Exception {
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+        String dataTable = generateUniqueName();
+        String indexName = "IDX_" + generateUniqueName();
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            conn.setAutoCommit(true);
+            String tableDDL = "CREATE TABLE " + dataTable + TestUtil.TEST_TABLE_SCHEMA;
+            conn.createStatement().execute(tableDDL);
+            BaseTest.upsertRows(conn, dataTable, 1);
+            String ddl = "CREATE INDEX " + indexName + " ON " + dataTable
+                    + " (varchar_col1 ASC, varchar_col2 ASC, int_pk DESC)"
+                    + " INCLUDE (int_col1, int_col2) ";
+            conn.createStatement().execute(ddl);
+
+            ResultSet rs = conn.getMetaData().getTables(null, null, indexName
+                    , new String[]{PTableType.INDEX.toString()});
+            assertTrue(rs.next());
+            assertEquals(indexName, rs.getString(3));
+            assertEquals(PIndexState.CREATE_DISABLE.toString(), rs.getString("INDEX_STATE"));
+            assertFalse(rs.next());
+
+            BaseTest.upsertRows(conn, dataTable, 3);
+            long rows = getRowCount(conn, indexName);
+            // Disabled table, runs doesn't go in

Review Comment:
   nit: rows don't go in



##########
phoenix-core/src/main/java/org/apache/phoenix/schema/PIndexState.java:
##########
@@ -32,7 +32,12 @@ public enum PIndexState {
     // When an index write fails, it is put in this state, and we let the client retry the mutation
     // After retries are exhausted, the client should mark the index as disabled, but if that
     // doesn't happen, then the index is considered disabled if it's been in this state too long
-    PENDING_DISABLE("w");
+    PENDING_DISABLE("w"),
+    //When we create/drop some indexes in one datacenter, the replication peer doesn't immediately have this index

Review Comment:
   nit: "in one cluster with a replication peer", not "in one datacenter"



##########
phoenix-core/src/main/java/org/apache/phoenix/index/IndexMaintainer.java:
##########
@@ -232,7 +232,15 @@ public static void serialize(PTable dataTable, ImmutableBytesWritable ptr,
         }
         int nIndexes = indexes.size();
         if (dataTable.getTransformingNewTable() != null) {
-            nIndexes++;
+            boolean disabled = dataTable.getTransformingNewTable().getIndexState()!= null &&
+                    dataTable.getTransformingNewTable().getIndexState().isDisabled();
+            if (disabled && nIndexes == 0) {
+                ptr.set(ByteUtil.EMPTY_BYTE_ARRAY);
+                return;
+            }
+            if (!disabled) {
+                nIndexes++;

Review Comment:
   Wouldn't this be incremented by the number of indexes on the transforming new table? Or is this one of those cases where we're treating the new table as an "index" to reuse the index maintainer code paths, and so it's only 1?



##########
phoenix-core/src/main/java/org/apache/phoenix/index/IndexMetaDataCacheClient.java:
##########
@@ -136,7 +137,11 @@ public static ServerCache setMetaDataOnMutations(PhoenixConnection connection, P
             }
             mutation.setAttribute(PhoenixIndexCodec.INDEX_UUID, uuidValue);
             if (table.getTransformingNewTable() != null) {
-                mutation.setAttribute(BaseScannerRegionObserver.DO_TRANSFORMING, TRUE_BYTES);
+                boolean disabled = table.getTransformingNewTable().getIndexState()!= null &&

Review Comment:
   since this is a very frequent check across several classes, should it be encapsulated as a function somewhere? (on PTable or in a Util class?)



##########
phoenix-core/src/main/java/org/apache/phoenix/index/IndexMaintainer.java:
##########
@@ -248,11 +256,15 @@ public static void serialize(PTable dataTable, ImmutableBytesWritable ptr,
                     output.write(protoBytes);
             }
             if (dataTable.getTransformingNewTable() != null) {
-                ServerCachingProtos.TransformMaintainer proto = TransformMaintainer.toProto(
-                        dataTable.getTransformingNewTable().getTransformMaintainer(dataTable, connection));
-                byte[] protoBytes = proto.toByteArray();
-                WritableUtils.writeVInt(output, protoBytes.length);
-                output.write(protoBytes);
+                boolean disabled = dataTable.getTransformingNewTable().getIndexState()!= null &&

Review Comment:
   likewise, comment here would be useful. Basically we're not serializing the transform maintainer is the new transformed table is still disabled?



##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexTwoPhaseCreateIT.java:
##########
@@ -0,0 +1,208 @@
+package org.apache.phoenix.end2end.index;
+
+import org.apache.hadoop.hbase.regionserver.ScanInfoUtil;
+import org.apache.phoenix.end2end.IndexToolIT;
+import org.apache.phoenix.end2end.transform.TransformToolIT;
+import org.apache.phoenix.jdbc.PhoenixConnection;
+import org.apache.phoenix.query.BaseTest;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.schema.PIndexState;
+import org.apache.phoenix.schema.PTable;
+import org.apache.phoenix.schema.PTableType;
+import org.apache.phoenix.schema.transform.SystemTransformRecord;
+import org.apache.phoenix.schema.transform.Transform;
+import org.apache.phoenix.thirdparty.com.google.common.collect.Maps;
+import org.apache.phoenix.util.PropertiesUtil;
+import org.apache.phoenix.util.ReadOnlyProps;
+import org.apache.phoenix.util.SchemaUtil;
+import org.apache.phoenix.util.StringUtil;
+import org.apache.phoenix.util.TestUtil;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Properties;
+
+import static org.apache.phoenix.query.QueryServices.INDEX_CREATE_DEFAULT_STATE;
+import static org.apache.phoenix.schema.PTable.ImmutableStorageScheme.ONE_CELL_PER_COLUMN;
+import static org.apache.phoenix.schema.PTable.QualifierEncodingScheme.NON_ENCODED_QUALIFIERS;
+import static org.apache.phoenix.util.TestUtil.INDEX_DATA_SCHEMA;
+import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
+import static org.apache.phoenix.util.TestUtil.getRowCount;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+public class IndexTwoPhaseCreateIT extends BaseTest {
+    @BeforeClass
+    public static synchronized void doSetup() throws Exception {
+        Map<String, String> props = Maps.newHashMapWithExpectedSize(1);
+        props.put(ScanInfoUtil.PHOENIX_MAX_LOOKBACK_AGE_CONF_KEY, Integer.toString(60 * 60)); // An hour
+        props.put(QueryServices.USE_STATS_FOR_PARALLELIZATION, Boolean.toString(false));
+        props.put(QueryServices.INDEX_CREATE_DEFAULT_STATE, PIndexState.CREATE_DISABLE.toString());
+        props.put(QueryServices.DEFAULT_IMMUTABLE_STORAGE_SCHEME_ATTRIB, "ONE_CELL_PER_COLUMN");
+        props.put(QueryServices.DEFAULT_COLUMN_ENCODED_BYTES_ATRRIB, "0");
+
+        setUpTestDriver(new ReadOnlyProps(props.entrySet().iterator()));
+    }
+
+    @AfterClass
+    public static synchronized void freeResources() throws Exception {
+        BaseTest.freeResourcesIfBeyondThreshold();
+    }
+
+    @Test
+    public void testIndexCreateWithNonDefaultSettings() throws Exception {
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+        String dataTable = generateUniqueName();
+        String indexName = "IDX_" + generateUniqueName();
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            String tableDDL = "CREATE TABLE " + dataTable + TestUtil.TEST_TABLE_SCHEMA;
+            conn.createStatement().execute(tableDDL);
+            BaseTest.upsertRows(conn, dataTable, 2);
+            String ddl = "CREATE INDEX " + indexName + " ON " + dataTable
+                    + " (varchar_col1 ASC, varchar_col2 ASC, int_pk DESC)"
+                    + " INCLUDE (int_col1, int_col2) ";
+            conn.createStatement().execute(ddl);
+
+            ResultSet rs = conn.getMetaData().getTables(null, null, indexName
+                    , new String[]{PTableType.INDEX.toString()});
+            assertTrue(rs.next());

Review Comment:
   Seems like you could extract a helper to validate a particular index is of a particular index state and save some duplicated code in this test case and several below. 



##########
phoenix-core/src/main/java/org/apache/phoenix/index/IndexMaintainer.java:
##########
@@ -232,7 +232,15 @@ public static void serialize(PTable dataTable, ImmutableBytesWritable ptr,
         }
         int nIndexes = indexes.size();
         if (dataTable.getTransformingNewTable() != null) {
-            nIndexes++;
+            boolean disabled = dataTable.getTransformingNewTable().getIndexState()!= null &&

Review Comment:
   Comment here would be useful



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org