You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by "ASF GitHub Bot (Jira)" <ji...@apache.org> on 2022/04/11 20:59:00 UTC
[jira] [Commented] (PHOENIX-6681) Enable new indexes to be optionally created in CREATE_DISABLED state
[ https://issues.apache.org/jira/browse/PHOENIX-6681?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17520809#comment-17520809 ]
ASF GitHub Bot commented on PHOENIX-6681:
-----------------------------------------
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
> Enable new indexes to be optionally created in CREATE_DISABLED state
> ---------------------------------------------------------------------
>
> Key: PHOENIX-6681
> URL: https://issues.apache.org/jira/browse/PHOENIX-6681
> Project: Phoenix
> Issue Type: New Feature
> Reporter: Gokcen Iskender
> Assignee: Gokcen Iskender
> Priority: Major
>
> When we create/drop some indexes in one datacenter, the replication pair doesn't immediately have this index and Hbase throws a replication error when we try to write into indexes that don't have a matching table on the replication pair.
> To remediate this issue, we can optionally create them in CREATE_DISABLED state and enable them after all the replication peers have the table.
> Same is true for dropping indexes. When they are dropped we can first disable them and drop them later.
--
This message was sent by Atlassian Jira
(v8.20.1#820001)