Uploaded image for project: 'Phoenix'
  1. Phoenix
  2. PHOENIX-3271

Distribute UPSERT SELECT across cluster

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.10.0
    • Labels:
      None

      Description

      Based on some informal testing we've done, it seems that creation of a local index is orders of magnitude faster that creation of global indexes (17 seconds versus 10-20 minutes - though more data is written in the global index case). Under the covers, a global index is created through the running of an UPSERT SELECT. Also, UPSERT SELECT provides an easy way of copying a table. In both of these cases, the data being upserted must all flow back to the same client which can become a bottleneck for a large table. Instead, what can be done is to push each separate, chunked UPSERT SELECT call out to a different region server for execution there. One way we could implement this would be to have an endpoint coprocessor push the chunked UPSERT SELECT out to each region server and return the number of rows that were upserted back to the client.

      1. PHOENIX-3271.patch
        15 kB
        Ankit Singhal
      2. PHOENIX-3271_v1.patch
        16 kB
        Ankit Singhal
      3. PHOENIX-3271_v2.patch
        21 kB
        Ankit Singhal
      4. PHOENIX-3271_v3.patch
        21 kB
        Ankit Singhal
      5. PHOENIX-3271_v4.patch
        22 kB
        Ankit Singhal
      6. PHOENIX-3271_v5.patch
        27 kB
        Ankit Singhal
      7. PHOENIX-3271_v5_rebased.patch
        26 kB
        Ankit Singhal

        Activity

        Hide
        samarthjain Samarth Jain added a comment -

        Is this different from the current optimization we have where UPSERT SELECT can run on server side? Should we just execute UPSERT SELECT on server side in most cases? If source and target tables are on different region servers, are cross region server RPCs going to be OK?

        Show
        samarthjain Samarth Jain added a comment - Is this different from the current optimization we have where UPSERT SELECT can run on server side? Should we just execute UPSERT SELECT on server side in most cases? If source and target tables are on different region servers, are cross region server RPCs going to be OK?
        Hide
        jamestaylor James Taylor added a comment -

        Yes, this is a different optimization. Running UPSERT SELECT on server side is only done when the source and target table are the same. This optimization is for the case when they're different, for example in the case of a global index build where the source table is the data table and the target table is the index table.

        Essentially, it'd take the parallel threads that are running per chunk on the client side and run them on different region servers to distribute the load.

        Show
        jamestaylor James Taylor added a comment - Yes, this is a different optimization. Running UPSERT SELECT on server side is only done when the source and target table are the same. This optimization is for the case when they're different, for example in the case of a global index build where the source table is the data table and the target table is the index table. Essentially, it'd take the parallel threads that are running per chunk on the client side and run them on different region servers to distribute the load.
        Hide
        jamestaylor James Taylor added a comment -

        FYI, Lars Hofhansl, Loknath Priyatham Teja Singamsetty - something to think about.

        Show
        jamestaylor James Taylor added a comment - FYI, Lars Hofhansl , Loknath Priyatham Teja Singamsetty - something to think about.
        Hide
        jamestaylor James Taylor added a comment -

        Thinking about it more, I think Samarth Jain's idea above is a good one. We can just always execute the UPSERT part from the server that handles the SELECT part to prevent all the data going back to the client. As long as we're batching appropriately, the RPC should be fine as the row isn't under lock.

        Ankit Singhal - would you be interested in this one?

        Show
        jamestaylor James Taylor added a comment - Thinking about it more, I think Samarth Jain 's idea above is a good one. We can just always execute the UPSERT part from the server that handles the SELECT part to prevent all the data going back to the client. As long as we're batching appropriately, the RPC should be fine as the row isn't under lock. Ankit Singhal - would you be interested in this one?
        Hide
        ankit@apache.org Ankit Singhal added a comment -

        In this case, we are assuming that SELECT statement should of a flat type which can be executed independently on the chunk and results be consumed for UPSERT directly.
        Like, the use case of GLOBAL INDEX building and copying table (with some filters), right?

        Show
        ankit@apache.org Ankit Singhal added a comment - In this case, we are assuming that SELECT statement should of a flat type which can be executed independently on the chunk and results be consumed for UPSERT directly. Like, the use case of GLOBAL INDEX building and copying table (with some filters), right?
        Hide
        jamestaylor James Taylor added a comment -

        Yes, that's correct, Ankit Singhal. We have a check for this in UpsertCompiler on the client side, so we'd want to relax the runServerSide logic to kick in even if the table name is different (but leave the other checks for post processing/merging in place so that execution still happens client side for these cases).

        Show
        jamestaylor James Taylor added a comment - Yes, that's correct, Ankit Singhal . We have a check for this in UpsertCompiler on the client side, so we'd want to relax the runServerSide logic to kick in even if the table name is different (but leave the other checks for post processing/merging in place so that execution still happens client side for these cases).
        Hide
        ankit@apache.org Ankit Singhal added a comment -

        James Taylor, can you please review.

        Show
        ankit@apache.org Ankit Singhal added a comment - James Taylor , can you please review.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12844043/PHOENIX-3271.patch
        against master branch at commit e45b5a706107e31bc6e5b8289725db097b2820eb.
        ATTACHMENT ID: 12844043

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 44 warning messages.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 lineLengths. The patch introduces the following lines longer than 100:
        + runOnServer = isAutoCommit && !table.isTransactional() && !(table.isImmutableRows() && !table.getIndexes().isEmpty()) && table.getRowTimestampColPos() == -1;
        + // If the row ends up living in a different region, we'll get an error otherwise.
        + .equals(new ColumnRef(tableRef, column.getPosition()).newColumnExpression())) {
        + // TODO: we could check the region boundaries to see if the pk will still be in it.
        + runOnServer = false; // bail on running server side, since PK may be changing
        + scan.setAttribute(BaseScannerRegionObserver.UPSERT_SELECT_TARGET_TABLE, tableRef.getTable().getPhysicalName().getBytes());
        + final QueryPlan aggPlan = new AggregatePlan(context, select, statementContext.getCurrentTable(), aggProjector, null,null, OrderBy.EMPTY_ORDER_BY, null, GroupBy.EMPTY_GROUP_BY, null);
        + private void commitBatchWithHTable(HTable table, Region region, List<Mutation> mutations, byte[] indexUUID,
        + long blockingMemstoreSize, byte[] indexMaintainersPtr, byte[] txState) throws IOException {
        + //Need to add indexMaintainers for each mutation as table.batch can be distributed across servers

        -1 core tests. The patch failed these unit tests:
        org.apache.phoenix.compile.QueryOptimizerTest
        org.apache.phoenix.util.PhoenixRuntimeTest
        org.apache.phoenix.index.IndexMaintainerTest

        Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/705//testReport/
        Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/705//artifact/patchprocess/patchJavadocWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/705//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12844043/PHOENIX-3271.patch against master branch at commit e45b5a706107e31bc6e5b8289725db097b2820eb. ATTACHMENT ID: 12844043 +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 javadoc . The javadoc tool appears to have generated 44 warning messages. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces the following lines longer than 100: + runOnServer = isAutoCommit && !table.isTransactional() && !(table.isImmutableRows() && !table.getIndexes().isEmpty()) && table.getRowTimestampColPos() == -1; + // If the row ends up living in a different region, we'll get an error otherwise. + .equals(new ColumnRef(tableRef, column.getPosition()).newColumnExpression())) { + // TODO: we could check the region boundaries to see if the pk will still be in it. + runOnServer = false; // bail on running server side, since PK may be changing + scan.setAttribute(BaseScannerRegionObserver.UPSERT_SELECT_TARGET_TABLE, tableRef.getTable().getPhysicalName().getBytes()); + final QueryPlan aggPlan = new AggregatePlan(context, select, statementContext.getCurrentTable(), aggProjector, null,null, OrderBy.EMPTY_ORDER_BY, null, GroupBy.EMPTY_GROUP_BY, null); + private void commitBatchWithHTable(HTable table, Region region, List<Mutation> mutations, byte[] indexUUID, + long blockingMemstoreSize, byte[] indexMaintainersPtr, byte[] txState) throws IOException { + //Need to add indexMaintainers for each mutation as table.batch can be distributed across servers -1 core tests . The patch failed these unit tests: org.apache.phoenix.compile.QueryOptimizerTest org.apache.phoenix.util.PhoenixRuntimeTest org.apache.phoenix.index.IndexMaintainerTest Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/705//testReport/ Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/705//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/705//console This message is automatically generated.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12844052/PHOENIX-3271_v1.patch
        against master branch at commit e45b5a706107e31bc6e5b8289725db097b2820eb.
        ATTACHMENT ID: 12844052

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 44 warning messages.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 lineLengths. The patch introduces the following lines longer than 100:
        + runOnServer = isAutoCommit && !table.isTransactional() && !(table.isImmutableRows() && !table.getIndexes().isEmpty()) && table.getRowTimestampColPos() == -1;
        + // If the row ends up living in a different region, we'll get an error otherwise.
        + .equals(new ColumnRef(tableRef, column.getPosition()).newColumnExpression())) {
        + // TODO: we could check the region boundaries to see if the pk will still be in it.
        + runOnServer = false; // bail on running server side, since PK may be changing
        + scan.setAttribute(BaseScannerRegionObserver.UPSERT_SELECT_TARGET_TABLE, tableRef.getTable().getPhysicalName().getBytes());
        + final QueryPlan aggPlan = new AggregatePlan(context, select, statementContext.getCurrentTable(), aggProjector, null,null, OrderBy.EMPTY_ORDER_BY, null, GroupBy.EMPTY_GROUP_BY, null);
        + private void commitBatchWithHTable(HTable table, Region region, List<Mutation> mutations, byte[] indexUUID,
        + long blockingMemstoreSize, byte[] indexMaintainersPtr, byte[] txState) throws IOException {
        + //Need to add indexMaintainers for each mutation as table.batch can be distributed across servers

        -1 core tests. The patch failed these unit tests:
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.SubqueryUsingSortMergeJoinIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.monitoring.PhoenixMetricsIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.DisableLocalIndexIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.DeleteIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ExecuteStatementsIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.SaltedViewIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.TenantSpecificViewIndexSaltedIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ViewIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.SortMergeJoinIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.PhoenixRuntimeIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.tx.TransactionIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.TenantSpecificTablesDMLIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.SubqueryIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.UpgradeIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.HashJoinIT

        Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/706//testReport/
        Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/706//artifact/patchprocess/patchJavadocWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/706//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12844052/PHOENIX-3271_v1.patch against master branch at commit e45b5a706107e31bc6e5b8289725db097b2820eb. ATTACHMENT ID: 12844052 +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 javadoc . The javadoc tool appears to have generated 44 warning messages. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces the following lines longer than 100: + runOnServer = isAutoCommit && !table.isTransactional() && !(table.isImmutableRows() && !table.getIndexes().isEmpty()) && table.getRowTimestampColPos() == -1; + // If the row ends up living in a different region, we'll get an error otherwise. + .equals(new ColumnRef(tableRef, column.getPosition()).newColumnExpression())) { + // TODO: we could check the region boundaries to see if the pk will still be in it. + runOnServer = false; // bail on running server side, since PK may be changing + scan.setAttribute(BaseScannerRegionObserver.UPSERT_SELECT_TARGET_TABLE, tableRef.getTable().getPhysicalName().getBytes()); + final QueryPlan aggPlan = new AggregatePlan(context, select, statementContext.getCurrentTable(), aggProjector, null,null, OrderBy.EMPTY_ORDER_BY, null, GroupBy.EMPTY_GROUP_BY, null); + private void commitBatchWithHTable(HTable table, Region region, List<Mutation> mutations, byte[] indexUUID, + long blockingMemstoreSize, byte[] indexMaintainersPtr, byte[] txState) throws IOException { + //Need to add indexMaintainers for each mutation as table.batch can be distributed across servers -1 core tests . The patch failed these unit tests: ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.SubqueryUsingSortMergeJoinIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.monitoring.PhoenixMetricsIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.DisableLocalIndexIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.DeleteIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ExecuteStatementsIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.SaltedViewIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.TenantSpecificViewIndexSaltedIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ViewIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.SortMergeJoinIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.PhoenixRuntimeIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.tx.TransactionIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.TenantSpecificTablesDMLIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.SubqueryIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.UpgradeIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.HashJoinIT Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/706//testReport/ Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/706//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/706//console This message is automatically generated.
        Hide
        ankit@apache.org Ankit Singhal added a comment -

        James Taylor, sorry for the spam.. you can ignore the patches, as it seems I need to take care of some more things to have all the test cases passed.

        Show
        ankit@apache.org Ankit Singhal added a comment - James Taylor , sorry for the spam.. you can ignore the patches, as it seems I need to take care of some more things to have all the test cases passed.
        Hide
        ankit@apache.org Ankit Singhal added a comment -

        Back on this today. Fixed the bugs with expressions arrangements and salting. now all the tests are passing.
        James Taylor, can you please review the latest patch.

        And, one more thing, we should restrict the parallelism(maybe one thread per region or by configuration hbase.htable.threads.max) to avoid the storm of writes in the cluster.(which I can do in follow up Jira). WDYT?

        Show
        ankit@apache.org Ankit Singhal added a comment - Back on this today. Fixed the bugs with expressions arrangements and salting. now all the tests are passing. James Taylor , can you please review the latest patch. And, one more thing, we should restrict the parallelism(maybe one thread per region or by configuration hbase.htable.threads.max) to avoid the storm of writes in the cluster.(which I can do in follow up Jira). WDYT?
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12846518/PHOENIX-3271_v3.patch
        against master branch at commit d8f4594989c0b73945aaffec5649a0b62ac59724.
        ATTACHMENT ID: 12846518

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 43 warning messages.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 lineLengths. The patch introduces the following lines longer than 100:
        + String ddl = "CREATE TABLE " + tableName1 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)"
        + ddl = "CREATE TABLE " + tableName2 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)"
        + projectedColumns.add(column.getPosition() == i + posOff ? column : new PColumnImpl(column, i));
        + final QueryPlan aggPlan = new AggregatePlan(context, select, statementContext.getCurrentTable(), aggProjector, null,null, OrderBy.EMPTY_ORDER_BY, null, GroupBy.EMPTY_GROUP_BY, null);
        + private void commitBatchWithHTable(HTable table, Region region, List<Mutation> mutations, byte[] indexUUID,
        + long blockingMemstoreSize, byte[] indexMaintainersPtr, byte[] txState) throws IOException {
        + // Need to add indexMaintainers for each mutation as table.batch can be distributed across servers
        + targetHTable = new HTable(env.getConfiguration(), projectedTable.getPhysicalName().getBytes());
        + region.getTableDesc().getTableName().getName()) == 0 && projectedTable.getRowTimestampColPos() == -1
        + commit(region, mutations, indexUUID, blockingMemStoreSize, indexMaintainersPtr, txState,

        -1 core tests. The patch failed these unit tests:

        Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/728//testReport/
        Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/728//artifact/patchprocess/patchJavadocWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/728//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12846518/PHOENIX-3271_v3.patch against master branch at commit d8f4594989c0b73945aaffec5649a0b62ac59724. ATTACHMENT ID: 12846518 +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 javadoc . The javadoc tool appears to have generated 43 warning messages. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces the following lines longer than 100: + String ddl = "CREATE TABLE " + tableName1 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)" + ddl = "CREATE TABLE " + tableName2 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)" + projectedColumns.add(column.getPosition() == i + posOff ? column : new PColumnImpl(column, i)); + final QueryPlan aggPlan = new AggregatePlan(context, select, statementContext.getCurrentTable(), aggProjector, null,null, OrderBy.EMPTY_ORDER_BY, null, GroupBy.EMPTY_GROUP_BY, null); + private void commitBatchWithHTable(HTable table, Region region, List<Mutation> mutations, byte[] indexUUID, + long blockingMemstoreSize, byte[] indexMaintainersPtr, byte[] txState) throws IOException { + // Need to add indexMaintainers for each mutation as table.batch can be distributed across servers + targetHTable = new HTable(env.getConfiguration(), projectedTable.getPhysicalName().getBytes()); + region.getTableDesc().getTableName().getName()) == 0 && projectedTable.getRowTimestampColPos() == -1 + commit(region, mutations, indexUUID, blockingMemStoreSize, indexMaintainersPtr, txState, -1 core tests . The patch failed these unit tests: Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/728//testReport/ Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/728//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/728//console This message is automatically generated.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12846510/PHOENIX-3271_v2.patch
        against master branch at commit d8f4594989c0b73945aaffec5649a0b62ac59724.
        ATTACHMENT ID: 12846510

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 43 warning messages.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 lineLengths. The patch introduces the following lines longer than 100:
        + String ddl = "CREATE TABLE " + tableName1 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)"
        + ddl = "CREATE TABLE " + tableName2 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)"
        + projectedColumns.add(column.getPosition() == i + posOff ? column : new PColumnImpl(column, i));
        + final QueryPlan aggPlan = new AggregatePlan(context, select, statementContext.getCurrentTable(), aggProjector, null,null, OrderBy.EMPTY_ORDER_BY, null, GroupBy.EMPTY_GROUP_BY, null);
        + private void commitBatchWithHTable(HTable table, Region region, List<Mutation> mutations, byte[] indexUUID,
        + long blockingMemstoreSize, byte[] indexMaintainersPtr, byte[] txState) throws IOException {
        + // Need to add indexMaintainers for each mutation as table.batch can be distributed across servers
        + targetHTable = new HTable(env.getConfiguration(), projectedTable.getPhysicalName().getBytes());
        + region.getTableDesc().getTableName().getName()) == 0 && projectedTable.getRowTimestampColPos() == -1
        + commit(region, mutations, indexUUID, blockingMemStoreSize, indexMaintainersPtr, txState,

        -1 core tests. The patch failed these unit tests:
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.IndexToolForPartialBuildWithNamespaceEnabledIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.UpsertSelectIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.IndexToolForPartialBuildIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.monitoring.PhoenixMetricsIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.ImmutableIndexIT

        Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/727//testReport/
        Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/727//artifact/patchprocess/patchJavadocWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/727//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12846510/PHOENIX-3271_v2.patch against master branch at commit d8f4594989c0b73945aaffec5649a0b62ac59724. ATTACHMENT ID: 12846510 +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 javadoc . The javadoc tool appears to have generated 43 warning messages. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces the following lines longer than 100: + String ddl = "CREATE TABLE " + tableName1 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)" + ddl = "CREATE TABLE " + tableName2 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)" + projectedColumns.add(column.getPosition() == i + posOff ? column : new PColumnImpl(column, i)); + final QueryPlan aggPlan = new AggregatePlan(context, select, statementContext.getCurrentTable(), aggProjector, null,null, OrderBy.EMPTY_ORDER_BY, null, GroupBy.EMPTY_GROUP_BY, null); + private void commitBatchWithHTable(HTable table, Region region, List<Mutation> mutations, byte[] indexUUID, + long blockingMemstoreSize, byte[] indexMaintainersPtr, byte[] txState) throws IOException { + // Need to add indexMaintainers for each mutation as table.batch can be distributed across servers + targetHTable = new HTable(env.getConfiguration(), projectedTable.getPhysicalName().getBytes()); + region.getTableDesc().getTableName().getName()) == 0 && projectedTable.getRowTimestampColPos() == -1 + commit(region, mutations, indexUUID, blockingMemStoreSize, indexMaintainersPtr, txState, -1 core tests . The patch failed these unit tests: ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.IndexToolForPartialBuildWithNamespaceEnabledIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.UpsertSelectIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.IndexToolForPartialBuildIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.monitoring.PhoenixMetricsIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.ImmutableIndexIT Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/727//testReport/ Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/727//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/727//console This message is automatically generated.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12846534/PHOENIX-3271_v3.patch
        against master branch at commit d8f4594989c0b73945aaffec5649a0b62ac59724.
        ATTACHMENT ID: 12846534

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 43 warning messages.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 lineLengths. The patch introduces the following lines longer than 100:
        + String ddl = "CREATE TABLE " + tableName1 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)"
        + ddl = "CREATE TABLE " + tableName2 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)"
        + projectedColumns.add(column.getPosition() == i + posOff ? column : new PColumnImpl(column, i));
        + final QueryPlan aggPlan = new AggregatePlan(context, select, statementContext.getCurrentTable(), aggProjector, null,null, OrderBy.EMPTY_ORDER_BY, null, GroupBy.EMPTY_GROUP_BY, null);
        + private void commitBatchWithHTable(HTable table, Region region, List<Mutation> mutations, byte[] indexUUID,
        + long blockingMemstoreSize, byte[] indexMaintainersPtr, byte[] txState) throws IOException {
        + // Need to add indexMaintainers for each mutation as table.batch can be distributed across servers
        + targetHTable = new HTable(env.getConfiguration(), projectedTable.getPhysicalName().getBytes());
        + region.getTableDesc().getTableName().getName()) == 0 && projectedTable.getRowTimestampColPos() == -1
        + commit(region, mutations, indexUUID, blockingMemStoreSize, indexMaintainersPtr, txState,

        -1 core tests. The patch failed these unit tests:

        Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/730//testReport/
        Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/730//artifact/patchprocess/patchJavadocWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/730//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12846534/PHOENIX-3271_v3.patch against master branch at commit d8f4594989c0b73945aaffec5649a0b62ac59724. ATTACHMENT ID: 12846534 +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 javadoc . The javadoc tool appears to have generated 43 warning messages. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces the following lines longer than 100: + String ddl = "CREATE TABLE " + tableName1 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)" + ddl = "CREATE TABLE " + tableName2 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)" + projectedColumns.add(column.getPosition() == i + posOff ? column : new PColumnImpl(column, i)); + final QueryPlan aggPlan = new AggregatePlan(context, select, statementContext.getCurrentTable(), aggProjector, null,null, OrderBy.EMPTY_ORDER_BY, null, GroupBy.EMPTY_GROUP_BY, null); + private void commitBatchWithHTable(HTable table, Region region, List<Mutation> mutations, byte[] indexUUID, + long blockingMemstoreSize, byte[] indexMaintainersPtr, byte[] txState) throws IOException { + // Need to add indexMaintainers for each mutation as table.batch can be distributed across servers + targetHTable = new HTable(env.getConfiguration(), projectedTable.getPhysicalName().getBytes()); + region.getTableDesc().getTableName().getName()) == 0 && projectedTable.getRowTimestampColPos() == -1 + commit(region, mutations, indexUUID, blockingMemStoreSize, indexMaintainersPtr, txState, -1 core tests . The patch failed these unit tests: Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/730//testReport/ Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/730//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/730//console This message is automatically generated.
        Hide
        ankit@apache.org Ankit Singhal added a comment -

        ping James Taylor. can you please take a look , tests were passing locally.

        Show
        ankit@apache.org Ankit Singhal added a comment - ping James Taylor . can you please take a look , tests were passing locally.
        Hide
        samarthjain Samarth Jain added a comment -

        Ankit Singhal - one thing to make sure would be that the thread pool used by cross region server UPSERTs is different from the thread pool whose threads are doing the scans for SELECTs. Otherwise, it could lead to deadlock like scenarios.

        Show
        samarthjain Samarth Jain added a comment - Ankit Singhal - one thing to make sure would be that the thread pool used by cross region server UPSERTs is different from the thread pool whose threads are doing the scans for SELECTs. Otherwise, it could lead to deadlock like scenarios.
        Hide
        jamestaylor James Taylor added a comment -

        A cross region call will be higher priority when made from a region server, so this should prevent deadlocks. Would make sense to have a stress for this to confirm.

        Show
        jamestaylor James Taylor added a comment - A cross region call will be higher priority when made from a region server, so this should prevent deadlocks. Would make sense to have a stress for this to confirm.
        Hide
        ankit@apache.org Ankit Singhal added a comment -

        with default configuration(assuming read and writes are sharing the same handler queue), I think deadlock can happen when all the region server handlers are saturated by the no. of working scans (on different chunks of the region) and there is no handler to handle the write. In this way, both will not succeed even with high priority writes as they need to wait for scan RPC completes.

        I'll try to do the stress test tomorrow to see if there are other cases.

        Show
        ankit@apache.org Ankit Singhal added a comment - with default configuration(assuming read and writes are sharing the same handler queue), I think deadlock can happen when all the region server handlers are saturated by the no. of working scans (on different chunks of the region) and there is no handler to handle the write. In this way, both will not succeed even with high priority writes as they need to wait for scan RPC completes. I'll try to do the stress test tomorrow to see if there are other cases.
        Hide
        jamestaylor James Taylor added a comment -

        The writes are higher priority based on the PhoenixRpcSchedulerFactory setup this configuration: https://phoenix.apache.org/secondary_indexing.html#Setup

        We'll want to document this and require that this scheduler factory is in place as this is what will prevent deadlocks. Perhaps on the initial client connection this setting can be verified.

        Show
        jamestaylor James Taylor added a comment - The writes are higher priority based on the PhoenixRpcSchedulerFactory setup this configuration: https://phoenix.apache.org/secondary_indexing.html#Setup We'll want to document this and require that this scheduler factory is in place as this is what will prevent deadlocks. Perhaps on the initial client connection this setting can be verified.
        Hide
        ankit@apache.org Ankit Singhal added a comment -

        I did some stress testing if no. of handlers are adequate, I have not seen any problem. should we commit this now?

        Show
        ankit@apache.org Ankit Singhal added a comment - I did some stress testing if no. of handlers are adequate, I have not seen any problem. should we commit this now?
        Hide
        jamestaylor James Taylor added a comment -

        Patch looks good, Ankit Singhal. Can you confirm that the cross RS calls are made with a higher priority? I think they are, but I think we should confirm (preferably with a new unit test - see PhoenixServerRpcIT and PhoenixClientRpcIT as potential places to add that).

        Show
        jamestaylor James Taylor added a comment - Patch looks good, Ankit Singhal . Can you confirm that the cross RS calls are made with a higher priority? I think they are, but I think we should confirm (preferably with a new unit test - see PhoenixServerRpcIT and PhoenixClientRpcIT as potential places to add that).
        Hide
        ankit@apache.org Ankit Singhal added a comment - - edited

        Thank you James Taylor for the review, Yes, Cross RPC calls other than system tables are getting dispatched through Index handlers only. As you said, there will not be a deadlock if this is set properly at the server.
        Have updated the v4 patch to include the test case for the same.

        thanks Devaraj Das for detailing that we have a separate pool created as a part of PhoenixRpcScheduler

        Show
        ankit@apache.org Ankit Singhal added a comment - - edited Thank you James Taylor for the review, Yes, Cross RPC calls other than system tables are getting dispatched through Index handlers only. As you said, there will not be a deadlock if this is set properly at the server. Have updated the v4 patch to include the test case for the same. thanks Devaraj Das for detailing that we have a separate pool created as a part of PhoenixRpcScheduler
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12849076/PHOENIX-3271_v4.patch
        against master branch at commit b9323e1d30ba6b449f059b86ae7b8157de16b13d.
        ATTACHMENT ID: 12849076

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 43 warning messages.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 lineLengths. The patch introduces the following lines longer than 100:
        + String ddl = "CREATE TABLE " + tableName1 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)"
        + ddl = "CREATE TABLE " + tableName2 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)"
        + "CREATE INDEX " + indexName + "_1 ON " + dataTableFullName + " (v1) INCLUDE (v2)");
        + // verify that that index queue is used and only once (during Upsert Select on server to build the index)
        + Mockito.verify(TestPhoenixIndexRpcSchedulerFactory.getIndexRpcExecutor()).dispatch(Mockito.any(CallRunner.class));
        + projectedColumns.add(column.getPosition() == i + posOff ? column : new PColumnImpl(column, i));
        + final QueryPlan aggPlan = new AggregatePlan(context, select, statementContext.getCurrentTable(), aggProjector, null,null, OrderBy.EMPTY_ORDER_BY, null, GroupBy.EMPTY_GROUP_BY, null);
        + private void commitBatchWithHTable(HTable table, Region region, List<Mutation> mutations, byte[] indexUUID,
        + long blockingMemstoreSize, byte[] indexMaintainersPtr, byte[] txState) throws IOException {
        + // Need to add indexMaintainers for each mutation as table.batch can be distributed across servers

        -1 core tests. The patch failed these unit tests:
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.IndexToolForPartialBuildWithNamespaceEnabledIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.UpsertSelectIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.IndexToolForPartialBuildIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.monitoring.PhoenixMetricsIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.ImmutableIndexIT

        Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/741//testReport/
        Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/741//artifact/patchprocess/patchJavadocWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/741//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12849076/PHOENIX-3271_v4.patch against master branch at commit b9323e1d30ba6b449f059b86ae7b8157de16b13d. ATTACHMENT ID: 12849076 +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 javadoc . The javadoc tool appears to have generated 43 warning messages. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces the following lines longer than 100: + String ddl = "CREATE TABLE " + tableName1 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)" + ddl = "CREATE TABLE " + tableName2 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)" + "CREATE INDEX " + indexName + "_1 ON " + dataTableFullName + " (v1) INCLUDE (v2)"); + // verify that that index queue is used and only once (during Upsert Select on server to build the index) + Mockito.verify(TestPhoenixIndexRpcSchedulerFactory.getIndexRpcExecutor()).dispatch(Mockito.any(CallRunner.class)); + projectedColumns.add(column.getPosition() == i + posOff ? column : new PColumnImpl(column, i)); + final QueryPlan aggPlan = new AggregatePlan(context, select, statementContext.getCurrentTable(), aggProjector, null,null, OrderBy.EMPTY_ORDER_BY, null, GroupBy.EMPTY_GROUP_BY, null); + private void commitBatchWithHTable(HTable table, Region region, List<Mutation> mutations, byte[] indexUUID, + long blockingMemstoreSize, byte[] indexMaintainersPtr, byte[] txState) throws IOException { + // Need to add indexMaintainers for each mutation as table.batch can be distributed across servers -1 core tests . The patch failed these unit tests: ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.IndexToolForPartialBuildWithNamespaceEnabledIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.UpsertSelectIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.IndexToolForPartialBuildIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.monitoring.PhoenixMetricsIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.ImmutableIndexIT Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/741//testReport/ Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/741//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/741//console This message is automatically generated.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12849141/PHOENIX-3271_v5.patch
        against master branch at commit b9323e1d30ba6b449f059b86ae7b8157de16b13d.
        ATTACHMENT ID: 12849141

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 43 warning messages.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 lineLengths. The patch introduces the following lines longer than 100:
        + String ddl = "CREATE TABLE " + tableName1 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)"
        + ddl = "CREATE TABLE " + tableName2 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)"
        + "CREATE INDEX " + indexName + "_1 ON " + dataTableFullName + " (v1) INCLUDE (v2)");
        + // verify that that index queue is used and only once (during Upsert Select on server to build the index)
        + Mockito.verify(TestPhoenixIndexRpcSchedulerFactory.getIndexRpcExecutor()).dispatch(Mockito.any(CallRunner.class));
        + projectedColumns.add(column.getPosition() == i + posOff ? column : new PColumnImpl(column, i));
        + // Hack to add default column family to be used on server in case no value column is projected.
        + final QueryPlan aggPlan = new AggregatePlan(context, select, statementContext.getCurrentTable(), aggProjector, null,null, OrderBy.EMPTY_ORDER_BY, null, GroupBy.EMPTY_GROUP_BY, null);
        + private void commitBatchWithHTable(HTable table, Region region, List<Mutation> mutations, byte[] indexUUID,
        + long blockingMemstoreSize, byte[] indexMaintainersPtr, byte[] txState) throws IOException {

        -1 core tests. The patch failed these unit tests:

        Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/742//testReport/
        Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/742//artifact/patchprocess/patchJavadocWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/742//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12849141/PHOENIX-3271_v5.patch against master branch at commit b9323e1d30ba6b449f059b86ae7b8157de16b13d. ATTACHMENT ID: 12849141 +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 javadoc . The javadoc tool appears to have generated 43 warning messages. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces the following lines longer than 100: + String ddl = "CREATE TABLE " + tableName1 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)" + ddl = "CREATE TABLE " + tableName2 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)" + "CREATE INDEX " + indexName + "_1 ON " + dataTableFullName + " (v1) INCLUDE (v2)"); + // verify that that index queue is used and only once (during Upsert Select on server to build the index) + Mockito.verify(TestPhoenixIndexRpcSchedulerFactory.getIndexRpcExecutor()).dispatch(Mockito.any(CallRunner.class)); + projectedColumns.add(column.getPosition() == i + posOff ? column : new PColumnImpl(column, i)); + // Hack to add default column family to be used on server in case no value column is projected. + final QueryPlan aggPlan = new AggregatePlan(context, select, statementContext.getCurrentTable(), aggProjector, null,null, OrderBy.EMPTY_ORDER_BY, null, GroupBy.EMPTY_GROUP_BY, null); + private void commitBatchWithHTable(HTable table, Region region, List<Mutation> mutations, byte[] indexUUID, + long blockingMemstoreSize, byte[] indexMaintainersPtr, byte[] txState) throws IOException { -1 core tests . The patch failed these unit tests: Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/742//testReport/ Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/742//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/742//console This message is automatically generated.
        Hide
        enis Enis Soztutar added a comment -

        This is a nice improvement.
        Rajeshbabu Chintaguntla has a patch which changes the rpc scheduler to be configured programmatically from the server side, related to PHOENIX-3360. Do we need that patch in before this?

        For this patch in general, depending on the handler priorities proved to be brittle, however this will work if we confirm that the index rpc handlers will be used in all cross-RS communication. Agreed that we have to fix documentation, and also rename "index" handlers. For the long term, I would rather have another approach, where the Phoenix Rpc scheduler has a different thread pool (with low priority) to execute generic "tasks". In this case, the scan fragments will be executed from that task thread pool, but the upsert writes go to the normal thread pool. Doing these scans with inserts kind of thing should not be piggy-backed on the scan flow I think.

        One other thing is that these scan RPCs will take a longer time and will timeout, and retried from the client, causing the worst case behavior to be pretty bad user experience. Do we have any plans for dealing with that? On the newer HBase's the scanner has heartbeats and can return earlier close to the scanner lease timeout. Does it apply for these upsert selects?

        Maybe we should add a safe-guard configuration in case, larger clusters cannot execute the scan fragments under rpc timeout. wdyt?

        Show
        enis Enis Soztutar added a comment - This is a nice improvement. Rajeshbabu Chintaguntla has a patch which changes the rpc scheduler to be configured programmatically from the server side, related to PHOENIX-3360 . Do we need that patch in before this? For this patch in general, depending on the handler priorities proved to be brittle, however this will work if we confirm that the index rpc handlers will be used in all cross-RS communication. Agreed that we have to fix documentation, and also rename "index" handlers. For the long term, I would rather have another approach, where the Phoenix Rpc scheduler has a different thread pool (with low priority) to execute generic "tasks". In this case, the scan fragments will be executed from that task thread pool, but the upsert writes go to the normal thread pool. Doing these scans with inserts kind of thing should not be piggy-backed on the scan flow I think. One other thing is that these scan RPCs will take a longer time and will timeout, and retried from the client, causing the worst case behavior to be pretty bad user experience. Do we have any plans for dealing with that? On the newer HBase's the scanner has heartbeats and can return earlier close to the scanner lease timeout. Does it apply for these upsert selects? Maybe we should add a safe-guard configuration in case, larger clusters cannot execute the scan fragments under rpc timeout. wdyt?
        Hide
        jamestaylor James Taylor added a comment - - edited

        I like your long term ideas, Enis Soztutar (JIRA please?), but I think this patch is good in the near term. The timeouts should be prevented by our RenewLease client side impl (by Samarth Jain). If HBase let us renew leases on the server side, that'd be an improvement, but what we have works.

        IMHO, having a safeguard config would lead to code duplication and make maintenance harder. I think we're ok without it (provided we do adequate testing). This patch should improve global index build times substantially.

        Show
        jamestaylor James Taylor added a comment - - edited I like your long term ideas, Enis Soztutar (JIRA please?), but I think this patch is good in the near term. The timeouts should be prevented by our RenewLease client side impl (by Samarth Jain ). If HBase let us renew leases on the server side, that'd be an improvement, but what we have works. IMHO, having a safeguard config would lead to code duplication and make maintenance harder. I think we're ok without it (provided we do adequate testing). This patch should improve global index build times substantially.
        Hide
        ankit@apache.org Ankit Singhal added a comment -

        Thanks Enis Soztutar for looking into this.

        Rajeshbabu Chintaguntla has a patch which changes the RPC scheduler to be configured programmatically from the server side, related to PHOENIX-3360. Do we need that patch in before this?

        I think yes because now it is easy to get in deadlock if our RPC scheduler is not present. So, loading it automatically on region server(using Rajeshbabu Chintaguntla short term fix as per Devaraj Das comment ) instead of depending on the user to explicitly set the configuration would be better. Rajeshbabu Chintaguntla, can you push the same as part of new JIRA if PHOENIX-3360 needs to be kept open for long term fix.

        One other thing is that these scan RPCs will take a longer time and will timeout, and retried from the client, causing the worst case behaviour to be pretty bad user experience. Do we have any plans for dealing with that?

        As James Taylor said, we should not expect the RPC timeout after Samarth Jain implementation(PHOENIX-2357).

        can I push this patch now if no further comments are there. James Taylor/Enis Soztutar?

        Post this, I'll take up the long-term fixes in Scheduler suggested by Enis Soztutar as a part of another JIRA.

        Show
        ankit@apache.org Ankit Singhal added a comment - Thanks Enis Soztutar for looking into this. Rajeshbabu Chintaguntla has a patch which changes the RPC scheduler to be configured programmatically from the server side, related to PHOENIX-3360 . Do we need that patch in before this? I think yes because now it is easy to get in deadlock if our RPC scheduler is not present. So, loading it automatically on region server(using Rajeshbabu Chintaguntla short term fix as per Devaraj Das comment ) instead of depending on the user to explicitly set the configuration would be better. Rajeshbabu Chintaguntla , can you push the same as part of new JIRA if PHOENIX-3360 needs to be kept open for long term fix. One other thing is that these scan RPCs will take a longer time and will timeout, and retried from the client, causing the worst case behaviour to be pretty bad user experience. Do we have any plans for dealing with that? As James Taylor said, we should not expect the RPC timeout after Samarth Jain implementation( PHOENIX-2357 ). can I push this patch now if no further comments are there. James Taylor / Enis Soztutar ? Post this, I'll take up the long-term fixes in Scheduler suggested by Enis Soztutar as a part of another JIRA.
        Hide
        jamestaylor James Taylor added a comment -

        +1 to push now provided you've verified that with RPC scheduler config in place that RS -> RS calls are high priority and that you've done a verification on a real cluster.

        Show
        jamestaylor James Taylor added a comment - +1 to push now provided you've verified that with RPC scheduler config in place that RS -> RS calls are high priority and that you've done a verification on a real cluster.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12849916/PHOENIX-3271_v5_rebased.patch
        against master branch at commit 069c371aec23a6b87519615eda7eafb895cb1af0.
        ATTACHMENT ID: 12849916

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 42 warning messages.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 lineLengths. The patch introduces the following lines longer than 100:
        + String ddl = "CREATE TABLE " + tableName1 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)"
        + ddl = "CREATE TABLE " + tableName2 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)"
        + "CREATE INDEX " + indexName + "_1 ON " + dataTableFullName + " (v1) INCLUDE (v2)");
        + // verify that that index queue is used and only once (during Upsert Select on server to build the index)
        + Mockito.verify(TestPhoenixIndexRpcSchedulerFactory.getIndexRpcExecutor()).dispatch(Mockito.any(CallRunner.class));
        + projectedColumns.add(column.getPosition() == i + posOff ? column : new PColumnImpl(column, i));
        + // Hack to add default column family to be used on server in case no value column is projected.
        + final QueryPlan aggPlan = new AggregatePlan(context, select, statementContext.getCurrentTable(), aggProjector, null,null, OrderBy.EMPTY_ORDER_BY, null, GroupBy.EMPTY_GROUP_BY, null);
        + private void commitBatchWithHTable(HTable table, Region region, List<Mutation> mutations, byte[] indexUUID,
        + long blockingMemstoreSize, byte[] indexMaintainersPtr, byte[] txState) throws IOException {

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/747//testReport/
        Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/747//artifact/patchprocess/patchJavadocWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/747//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12849916/PHOENIX-3271_v5_rebased.patch against master branch at commit 069c371aec23a6b87519615eda7eafb895cb1af0. ATTACHMENT ID: 12849916 +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 javadoc . The javadoc tool appears to have generated 42 warning messages. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces the following lines longer than 100: + String ddl = "CREATE TABLE " + tableName1 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)" + ddl = "CREATE TABLE " + tableName2 + " (K BIGINT NOT NULL PRIMARY KEY ROW_TIMESTAMP, V VARCHAR)" + "CREATE INDEX " + indexName + "_1 ON " + dataTableFullName + " (v1) INCLUDE (v2)"); + // verify that that index queue is used and only once (during Upsert Select on server to build the index) + Mockito.verify(TestPhoenixIndexRpcSchedulerFactory.getIndexRpcExecutor()).dispatch(Mockito.any(CallRunner.class)); + projectedColumns.add(column.getPosition() == i + posOff ? column : new PColumnImpl(column, i)); + // Hack to add default column family to be used on server in case no value column is projected. + final QueryPlan aggPlan = new AggregatePlan(context, select, statementContext.getCurrentTable(), aggProjector, null,null, OrderBy.EMPTY_ORDER_BY, null, GroupBy.EMPTY_GROUP_BY, null); + private void commitBatchWithHTable(HTable table, Region region, List<Mutation> mutations, byte[] indexUUID, + long blockingMemstoreSize, byte[] indexMaintainersPtr, byte[] txState) throws IOException { +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/747//testReport/ Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/747//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/747//console This message is automatically generated.
        Hide
        ankit@apache.org Ankit Singhal added a comment -

        Thanks James Taylor

        +1 to push now provided you've verified that with RPC scheduler config in place that RS -> RS calls are high priority and that you've done a verification on a real cluster.

        Yes, this was verified.

        ping Rajeshbabu Chintaguntla for committing short term fix to load PhoenixRPCScheduler at server side automatically.
        Committed this to master and 4.x branches.

        Show
        ankit@apache.org Ankit Singhal added a comment - Thanks James Taylor +1 to push now provided you've verified that with RPC scheduler config in place that RS -> RS calls are high priority and that you've done a verification on a real cluster. Yes, this was verified. ping Rajeshbabu Chintaguntla for committing short term fix to load PhoenixRPCScheduler at server side automatically. Committed this to master and 4.x branches.
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Jenkins build Phoenix-master #1543 (See https://builds.apache.org/job/Phoenix-master/1543/)
        PHOENIX-3271 Distribute UPSERT SELECT across cluster (ankitsinghal59: rev 39460bc470d77f173d40b17f87a82c259bff5027)

        • (edit) phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java
        • (edit) phoenix-core/src/it/java/org/apache/phoenix/rpc/PhoenixServerRpcIT.java
        • (edit) phoenix-core/src/main/java/org/apache/phoenix/compile/UpsertCompiler.java
        • (edit) phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixMetricsIT.java
        • (edit) phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Jenkins build Phoenix-master #1543 (See https://builds.apache.org/job/Phoenix-master/1543/ ) PHOENIX-3271 Distribute UPSERT SELECT across cluster (ankitsinghal59: rev 39460bc470d77f173d40b17f87a82c259bff5027) (edit) phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java (edit) phoenix-core/src/it/java/org/apache/phoenix/rpc/PhoenixServerRpcIT.java (edit) phoenix-core/src/main/java/org/apache/phoenix/compile/UpsertCompiler.java (edit) phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixMetricsIT.java (edit) phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java
        Hide
        lhofhansl Lars Hofhansl added a comment -

        When we seed an index it will start with a single region, right? Are are we making sure that sure that all the region server with data for the table do not hammer that one poor region server that has the initial region and making it unusable?

        Show
        lhofhansl Lars Hofhansl added a comment - When we seed an index it will start with a single region, right? Are are we making sure that sure that all the region server with data for the table do not hammer that one poor region server that has the initial region and making it unusable?
        Hide
        lhofhansl Lars Hofhansl added a comment -
        Show
        lhofhansl Lars Hofhansl added a comment - Any comment on this? Ankit Singhal , James Taylor , Rajeshbabu Chintaguntla
        Hide
        jamestaylor James Taylor added a comment -

        I filed PHOENIX-3738 for any additional testing that needs to be done and PHOENIX-3737 for potentially throttling the initial writes (while we're still writing to only a few regions).

        When an index is built asynchronously, we're essentially already doing this. It'll end up being throttled by the number of mappers that are allowed.

        Show
        jamestaylor James Taylor added a comment - I filed PHOENIX-3738 for any additional testing that needs to be done and PHOENIX-3737 for potentially throttling the initial writes (while we're still writing to only a few regions). When an index is built asynchronously, we're essentially already doing this. It'll end up being throttled by the number of mappers that are allowed.

          People

          • Assignee:
            ankit@apache.org Ankit Singhal
            Reporter:
            jamestaylor James Taylor
          • Votes:
            0 Vote for this issue
            Watchers:
            11 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development