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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
