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

Make use of HBASE-15600 to write local index mutations along with data mutations atomically

    Details

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

      Description

      After HBASE-15600 we can add mutations of the same table from coprocessors so we can write local index data along with data mutations so it will be atomic. This we can do in 4.x-HBase-1.3 version.

      1. PHOENIX-3827.patch
        5 kB
        Rajeshbabu Chintaguntla
      2. PHOENIX-3827_v2.patch
        11 kB
        Rajeshbabu Chintaguntla
      3. PHOENIX-3827_v3.patch
        20 kB
        James Taylor
      4. PHOENIX-3827_v4.patch
        13 kB
        Rajeshbabu Chintaguntla
      5. PHOENIX-3827_v5.patch
        15 kB
        Rajeshbabu Chintaguntla

        Issue Links

          Activity

          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/12866823/PHOENIX-3827.patch
          against master branch at commit f51c0db9f2d2ee261e602a114d47dd63353bbba8.
          ATTACHMENT ID: 12866823

          +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 47 warning messages.

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

          +1 lineLengths. The patch does not introduce lines longer than 100

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

          Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/847//testReport/
          Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/847//artifact/patchprocess/patchJavadocWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/847//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/12866823/PHOENIX-3827.patch against master branch at commit f51c0db9f2d2ee261e602a114d47dd63353bbba8. ATTACHMENT ID: 12866823 +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 47 warning messages. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 -1 core tests . The patch failed these unit tests: Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/847//testReport/ Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/847//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/847//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/12866835/PHOENIX-3827_v2.patch
          against master branch at commit f51c0db9f2d2ee261e602a114d47dd63353bbba8.
          ATTACHMENT ID: 12866835

          +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 patch appears to cause mvn compile goal to fail .

          Compilation errors resume:
          [ERROR] COMPILATION ERROR :
          [ERROR] /home/jenkins/jenkins-slave/workspace/PreCommit-PHOENIX-Build/phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolForPartialBuildWithNamespaceEnabledIT.java:[43,9] constructor IndexToolForPartialBuildIT in class org.apache.phoenix.end2end.IndexToolForPartialBuildIT cannot be applied to given types;
          [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.0:testCompile (default-testCompile) on project phoenix-core: Compilation failure
          [ERROR] /home/jenkins/jenkins-slave/workspace/PreCommit-PHOENIX-Build/phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolForPartialBuildWithNamespaceEnabledIT.java:[43,9] constructor IndexToolForPartialBuildIT in class org.apache.phoenix.end2end.IndexToolForPartialBuildIT cannot be applied to given types;
          [ERROR] required: no arguments
          [ERROR] found: boolean
          [ERROR] reason: actual and formal argument lists differ in length
          [ERROR] -> [Help 1]
          [ERROR]
          [ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
          [ERROR] Re-run Maven using the -X switch to enable full debug logging.
          [ERROR]
          [ERROR] For more information about the errors and possible solutions, please read the following articles:
          [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
          [ERROR]
          [ERROR] After correcting the problems, you can resume the build with the command
          [ERROR] mvn <goals> -rf :phoenix-core

          Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/848//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/12866835/PHOENIX-3827_v2.patch against master branch at commit f51c0db9f2d2ee261e602a114d47dd63353bbba8. ATTACHMENT ID: 12866835 +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 patch appears to cause mvn compile goal to fail . Compilation errors resume: [ERROR] COMPILATION ERROR : [ERROR] /home/jenkins/jenkins-slave/workspace/PreCommit-PHOENIX-Build/phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolForPartialBuildWithNamespaceEnabledIT.java: [43,9] constructor IndexToolForPartialBuildIT in class org.apache.phoenix.end2end.IndexToolForPartialBuildIT cannot be applied to given types; [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.0:testCompile (default-testCompile) on project phoenix-core: Compilation failure [ERROR] /home/jenkins/jenkins-slave/workspace/PreCommit-PHOENIX-Build/phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolForPartialBuildWithNamespaceEnabledIT.java: [43,9] constructor IndexToolForPartialBuildIT in class org.apache.phoenix.end2end.IndexToolForPartialBuildIT cannot be applied to given types; [ERROR] required: no arguments [ERROR] found: boolean [ERROR] reason: actual and formal argument lists differ in length [ERROR] -> [Help 1] [ERROR] [ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch. [ERROR] Re-run Maven using the -X switch to enable full debug logging. [ERROR] [ERROR] For more information about the errors and possible solutions, please read the following articles: [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException [ERROR] [ERROR] After correcting the problems, you can resume the build with the command [ERROR] mvn <goals> -rf :phoenix-core Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/848//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/12866843/PHOENIX-3827_v2.patch
          against master branch at commit f51c0db9f2d2ee261e602a114d47dd63353bbba8.
          ATTACHMENT ID: 12866843

          +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 47 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:
          + stmt.execute(String.format("CREATE INDEX %s ON %s (LPAD(UPPER(NAME),11,'x')||'_xyz') ", indxTable, fullTableName));
          + assertExplainPlan(actualExplainPlan, schemaName, dataTableName, indxTable, isNamespaceEnabled);

          -1 core tests. The patch failed these unit tests:
          ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.OnDuplicateKeyIT
          ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.ReadOnlyIndexFailureIT
          ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.AutomaticRebuildIT

          Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/849//testReport/
          Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/849//artifact/patchprocess/patchJavadocWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/849//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/12866843/PHOENIX-3827_v2.patch against master branch at commit f51c0db9f2d2ee261e602a114d47dd63353bbba8. ATTACHMENT ID: 12866843 +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 47 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: + stmt.execute(String.format("CREATE INDEX %s ON %s (LPAD(UPPER(NAME),11,'x')||'_xyz') ", indxTable, fullTableName)); + assertExplainPlan(actualExplainPlan, schemaName, dataTableName, indxTable, isNamespaceEnabled); -1 core tests . The patch failed these unit tests: ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.OnDuplicateKeyIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.ReadOnlyIndexFailureIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.AutomaticRebuildIT Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/849//testReport/ Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/849//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/849//console This message is automatically generated.
          Hide
          jamestaylor James Taylor added a comment -

          Thanks for the patch, Rajeshbabu Chintaguntla. Any reason why you need to get rid of those tests for local indexes?

          Show
          jamestaylor James Taylor added a comment - Thanks for the patch, Rajeshbabu Chintaguntla . Any reason why you need to get rid of those tests for local indexes?
          Hide
          jamestaylor James Taylor added a comment -

          Had an offline conversation with Rajeshbabu Chintaguntla - the test changes are necessary in the case where we're testing a local index becoming out of sync with the data table. Since this is no longer possible, these tests should either be adapted. If possible, it'd be good to adapt these tests (similar to what we did when the table is transactional) to confirm that the local index remains in sync with the data table (even across these kinds of failures). You might be able to just piggyback on the checks we do for isTransactional and change to isTransactional || isLocalIndex.

          As far as the OnDuplicateKeyIgnoreIT, I'm not sure why those are failing. When the ON DUPLICATE KEY syntax is used, these mutations come in through the preIncrementAfterRowLock hook as Increment from the the client side. In this hook, we transform the Increment to the Put/Delete mutations instead (after applying the ON DUPLICATE KEY logic) and call region.mutateRowsWithLocks (because we know the changes will all be region local). That should invoke the regular secondary index code path - the data table mutations should just generate the index table mutations. Any reason why this would be different for local indexes?

          Show
          jamestaylor James Taylor added a comment - Had an offline conversation with Rajeshbabu Chintaguntla - the test changes are necessary in the case where we're testing a local index becoming out of sync with the data table. Since this is no longer possible, these tests should either be adapted. If possible, it'd be good to adapt these tests (similar to what we did when the table is transactional) to confirm that the local index remains in sync with the data table (even across these kinds of failures). You might be able to just piggyback on the checks we do for isTransactional and change to isTransactional || isLocalIndex . As far as the OnDuplicateKeyIgnoreIT, I'm not sure why those are failing. When the ON DUPLICATE KEY syntax is used, these mutations come in through the preIncrementAfterRowLock hook as Increment from the the client side. In this hook, we transform the Increment to the Put/Delete mutations instead (after applying the ON DUPLICATE KEY logic) and call region.mutateRowsWithLocks (because we know the changes will all be region local). That should invoke the regular secondary index code path - the data table mutations should just generate the index table mutations. Any reason why this would be different for local indexes?
          Hide
          jamestaylor James Taylor added a comment -

          Rajeshbabu Chintaguntla - I debugged the OnDuplicateKeyIT failures a bit and I'm not sure what's going on. As mentioned before, the region.mutateRowsWithLocks() call from Indexer.preIncrementAfterRowLock() calls the regular Indexer coprocessor hooks, the new local index rows get generated and added as expected, but then they don't appear to be written. Not sure what's going on - would you mind taking a look?

          Show
          jamestaylor James Taylor added a comment - Rajeshbabu Chintaguntla - I debugged the OnDuplicateKeyIT failures a bit and I'm not sure what's going on. As mentioned before, the region.mutateRowsWithLocks() call from Indexer.preIncrementAfterRowLock() calls the regular Indexer coprocessor hooks, the new local index rows get generated and added as expected, but then they don't appear to be written. Not sure what's going on - would you mind taking a look?
          Hide
          jamestaylor James Taylor added a comment -

          Remove writing of local indexes by ParallelWriterIndexCommitter. Rajeshbabu Chintaguntla - we still need the test changes associated with this change (the v2 patch needs to be rebased), and we need to understand why the OnDuplicateKeyIT tests are failing. The local index rows are being added as expected using the new miniBatchOp method. It's called indirectly: originally through the preIncrementAfterRowLock which in turn does a mutateRowsWithLocks while calling bypass and complete on the original RegionCoprocessorEnvironment for the increment call. Is that a problem?

          Mujtaba Chohan - can you try this patch to see if it fixes PHOENIX-3853?

          Show
          jamestaylor James Taylor added a comment - Remove writing of local indexes by ParallelWriterIndexCommitter. Rajeshbabu Chintaguntla - we still need the test changes associated with this change (the v2 patch needs to be rebased), and we need to understand why the OnDuplicateKeyIT tests are failing. The local index rows are being added as expected using the new miniBatchOp method. It's called indirectly: originally through the preIncrementAfterRowLock which in turn does a mutateRowsWithLocks while calling bypass and complete on the original RegionCoprocessorEnvironment for the increment call. Is that a problem? Mujtaba Chohan - can you try this patch to see if it fixes PHOENIX-3853 ?
          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/12868401/PHOENIX-3827_v3.patch
          against master branch at commit 442d8eb29f1f73fd104a323d9aa77f3a4ccfd8d1.
          ATTACHMENT ID: 12868401

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

          +1 tests included. The patch appears to include 12 new or modified tests.

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

          -1 javadoc. The javadoc tool appears to have generated 46 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:
          + public void write(Collection<Pair<Mutation, byte[]>> toWrite) throws IndexWriteException, IOException {
          + public void write(Multimap<HTableInterfaceReference, Mutation> toWrite) throws SingleIndexWriteFailureException {
          + public void write(Multimap<HTableInterfaceReference, Mutation> toWrite) throws MultiIndexWriteFailureException {

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

          Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/870//testReport/
          Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/870//artifact/patchprocess/patchJavadocWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/870//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/12868401/PHOENIX-3827_v3.patch against master branch at commit 442d8eb29f1f73fd104a323d9aa77f3a4ccfd8d1. ATTACHMENT ID: 12868401 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 12 new or modified tests. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 javadoc . The javadoc tool appears to have generated 46 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: + public void write(Collection<Pair<Mutation, byte[]>> toWrite) throws IndexWriteException, IOException { + public void write(Multimap<HTableInterfaceReference, Mutation> toWrite) throws SingleIndexWriteFailureException { + public void write(Multimap<HTableInterfaceReference, Mutation> toWrite) throws MultiIndexWriteFailureException { -1 core tests . The patch failed these unit tests: Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/870//testReport/ Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/870//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/870//console This message is automatically generated.
          Hide
          mujtabachohan Mujtaba Chohan added a comment -

          James Taylor There is no change in performance with PHOENIX-3827_v3.patch and it remains the same as described in PHOENIX-3853

          Show
          mujtabachohan Mujtaba Chohan added a comment - James Taylor There is no change in performance with PHOENIX-3827 _v3.patch and it remains the same as described in PHOENIX-3853
          Hide
          rajeshbabu Rajeshbabu Chintaguntla added a comment -

          James Taylor I am working on finding the root cause of OnDuplicateKeyIT test failure but by the time after Mujtaba Chohan raised PHOENIX-3853 looking at the performance issue. I am wondering how the local indexes are slow.
          As for the patch we collect the local index updates by removing them from index updates if the table is same and add to the list of ongoing mutations. Then what HBase internally does it for every mutation in the batch it will get the corresponding index mutations acquire row lock and merge these cells to data mutations to write atomically(Then the remaining story is same as normal writes). I don't think this is costlier than normal writes. I have things to suspect
          1) while loading data observed splits which might have caused some slowness
          2) Or else index updates preparation might have been slow
          Continuing testing to find the root cause of slowness.

          Show
          rajeshbabu Rajeshbabu Chintaguntla added a comment - James Taylor I am working on finding the root cause of OnDuplicateKeyIT test failure but by the time after Mujtaba Chohan raised PHOENIX-3853 looking at the performance issue. I am wondering how the local indexes are slow. As for the patch we collect the local index updates by removing them from index updates if the table is same and add to the list of ongoing mutations. Then what HBase internally does it for every mutation in the batch it will get the corresponding index mutations acquire row lock and merge these cells to data mutations to write atomically(Then the remaining story is same as normal writes). I don't think this is costlier than normal writes. I have things to suspect 1) while loading data observed splits which might have caused some slowness 2) Or else index updates preparation might have been slow Continuing testing to find the root cause of slowness.
          Hide
          rajeshbabu Rajeshbabu Chintaguntla added a comment -

          James Taylor As part of HBASE-15600 in doMiniBatchMutate we are adding the KeyValues generated in preBatchMutate to data mutations but in case of mutateRowsWithLocks we are not doing the same that's why the local index mutations are not written to the table. Am going to fix by following the current approach of directly writing to region in case of ON DUPLICATE KEY case. wdyt?

          Show
          rajeshbabu Rajeshbabu Chintaguntla added a comment - James Taylor As part of HBASE-15600 in doMiniBatchMutate we are adding the KeyValues generated in preBatchMutate to data mutations but in case of mutateRowsWithLocks we are not doing the same that's why the local index mutations are not written to the table. Am going to fix by following the current approach of directly writing to region in case of ON DUPLICATE KEY case. wdyt?
          Hide
          jamestaylor James Taylor added a comment -

          The region.mutateRowsWithLocks() leads to the call of the regular Indexer coprocessor hooks, the new local index rows get generated and added as expected though your new miniBatchOp method, but then they don't appear to be written. Why wouldn't they be? We use mutateRowsWithLocks in UngroupedAggregateRegionObserver in a similar way, and everything seems to work correctly.

          Show
          jamestaylor James Taylor added a comment - The region.mutateRowsWithLocks() leads to the call of the regular Indexer coprocessor hooks, the new local index rows get generated and added as expected though your new miniBatchOp method, but then they don't appear to be written. Why wouldn't they be? We use mutateRowsWithLocks in UngroupedAggregateRegionObserver in a similar way, and everything seems to work correctly.
          Hide
          rajeshbabu Rajeshbabu Chintaguntla added a comment -

          James Taylor

          The region.mutateRowsWithLocks() leads to the call of the regular Indexer coprocessor hooks, the new local index rows get generated and added as expected though your new miniBatchOp method, but then they don't appear to be written. Why wouldn't they be?

          What ever mutations added by Indexer coprocessor hooks will be consumed in HRegion#doMiniBatchMutation but it's not the case in mutateRowsWithLocks. We need to raise an issue in HBase to support the same in mutateRowsWithLocks as well.

                    for (int i = firstIndex; i < lastIndexExclusive; i++) {
                      if (batchOp.retCodeDetails[i].getOperationStatusCode() != OperationStatusCode.NOT_RUN) {
                        // lastIndexExclusive was incremented above.
                        continue;
                      }
                      // we pass (i - firstIndex) below since the call expects a relative index
                      Mutation[] cpMutations = miniBatchOp.getOperationsFromCoprocessors(i - firstIndex);
                      if (cpMutations == null) {
                        continue;
                      }
                      // Else Coprocessor added more Mutations corresponding to the Mutation at this index.
                      for (int j = 0; j < cpMutations.length; j++) {
                        Mutation cpMutation = miniBatchOp.getOperationsFromCoprocessors(i)[j];
                        Map<byte[], List<Cell>> cpFamilyMap = cpMutation.getFamilyCellMap();
                        checkAndPrepareMutation(cpMutation, isInReplay, cpFamilyMap, now);
          
                        // Acquire row locks. If not, the whole batch will fail.
                        acquiredRowLocks.add(getRowLock(cpMutation.getRow(), true));
          
                        if (cpMutation.getDurability() == Durability.SKIP_WAL) {
                          recordMutationWithoutWal(cpFamilyMap);
                        }
          
                        // Returned mutations from coprocessor correspond to the Mutation at index i. We can
                        // directly add the cells from those mutations to the familyMaps of this mutation.
                        mergeFamilyMaps(familyMaps[i], cpFamilyMap); // will get added to the memstore later
                      }
          

          We use mutateRowsWithLocks in UngroupedAggregateRegionObserver in a similar way, and everything seems to work correctly.

          We are using HRegion#batchMutate only where this patch works perfectly.

          Show
          rajeshbabu Rajeshbabu Chintaguntla added a comment - James Taylor The region.mutateRowsWithLocks() leads to the call of the regular Indexer coprocessor hooks, the new local index rows get generated and added as expected though your new miniBatchOp method, but then they don't appear to be written. Why wouldn't they be? What ever mutations added by Indexer coprocessor hooks will be consumed in HRegion#doMiniBatchMutation but it's not the case in mutateRowsWithLocks. We need to raise an issue in HBase to support the same in mutateRowsWithLocks as well. for (int i = firstIndex; i < lastIndexExclusive; i++) { if (batchOp.retCodeDetails[i].getOperationStatusCode() != OperationStatusCode.NOT_RUN) { // lastIndexExclusive was incremented above. continue; } // we pass (i - firstIndex) below since the call expects a relative index Mutation[] cpMutations = miniBatchOp.getOperationsFromCoprocessors(i - firstIndex); if (cpMutations == null) { continue; } // Else Coprocessor added more Mutations corresponding to the Mutation at this index. for (int j = 0; j < cpMutations.length; j++) { Mutation cpMutation = miniBatchOp.getOperationsFromCoprocessors(i)[j]; Map<byte[], List<Cell>> cpFamilyMap = cpMutation.getFamilyCellMap(); checkAndPrepareMutation(cpMutation, isInReplay, cpFamilyMap, now); // Acquire row locks. If not, the whole batch will fail. acquiredRowLocks.add(getRowLock(cpMutation.getRow(), true)); if (cpMutation.getDurability() == Durability.SKIP_WAL) { recordMutationWithoutWal(cpFamilyMap); } // Returned mutations from coprocessor correspond to the Mutation at index i. We can // directly add the cells from those mutations to the familyMaps of this mutation. mergeFamilyMaps(familyMaps[i], cpFamilyMap); // will get added to the memstore later } We use mutateRowsWithLocks in UngroupedAggregateRegionObserver in a similar way, and everything seems to work correctly. We are using HRegion#batchMutate only where this patch works perfectly.
          Hide
          jamestaylor James Taylor added a comment -

          If it works to using HRegion#batchMutate, let's just switch to that. WDYT? Can you put up a new patch, Rajeshbabu Chintaguntla?

          Show
          jamestaylor James Taylor added a comment - If it works to using HRegion#batchMutate, let's just switch to that. WDYT? Can you put up a new patch, Rajeshbabu Chintaguntla ?
          Hide
          rajeshbabu Rajeshbabu Chintaguntla added a comment -

          Yes we can make use of HRegion#batchMutate and will submit new patch James Taylor

          Show
          rajeshbabu Rajeshbabu Chintaguntla added a comment - Yes we can make use of HRegion#batchMutate and will submit new patch James Taylor
          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/12869498/PHOENIX-3827_v4.patch
          against master branch at commit 4471b87926094b71aff12c384f4fc60f87ff2915.
          ATTACHMENT ID: 12869498

          +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 47 warning messages.

          -1 release audit. The applied patch generated 2 release audit warnings (more than the master's current 0 warnings).

          -1 lineLengths. The patch introduces the following lines longer than 100:
          + stmt.execute(String.format("CREATE INDEX %s ON %s (LPAD(UPPER(NAME),11,'x')||'_xyz') ", indxTable, fullTableName));
          + assertExplainPlan(actualExplainPlan, schemaName, dataTableName, indxTable, isNamespaceEnabled);

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

          Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/892//testReport/
          Release audit warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/892//artifact/patchprocess/patchReleaseAuditWarnings.txt
          Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/892//artifact/patchprocess/patchJavadocWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/892//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/12869498/PHOENIX-3827_v4.patch against master branch at commit 4471b87926094b71aff12c384f4fc60f87ff2915. ATTACHMENT ID: 12869498 +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 47 warning messages. -1 release audit . The applied patch generated 2 release audit warnings (more than the master's current 0 warnings). -1 lineLengths . The patch introduces the following lines longer than 100: + stmt.execute(String.format("CREATE INDEX %s ON %s (LPAD(UPPER(NAME),11,'x')||'_xyz') ", indxTable, fullTableName)); + assertExplainPlan(actualExplainPlan, schemaName, dataTableName, indxTable, isNamespaceEnabled); -1 core tests . The patch failed these unit tests: Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/892//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/892//artifact/patchprocess/patchReleaseAuditWarnings.txt Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/892//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/892//console This message is automatically generated.
          Hide
          jamestaylor James Taylor added a comment -

          +1 on patch for master. Thanks, Rajeshbabu Chintaguntla! One oddity is that not all tests seem to have run - for example, the end2end.index tests took 3.2 seconds. Not sure why that is - maybe you can confirm that tests pass locally and then check it in.

          Show
          jamestaylor James Taylor added a comment - +1 on patch for master. Thanks, Rajeshbabu Chintaguntla ! One oddity is that not all tests seem to have run - for example, the end2end.index tests took 3.2 seconds. Not sure why that is - maybe you can confirm that tests pass locally and then check it in.
          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/12869503/PHOENIX-3827_v4.patch
          against master branch at commit 4471b87926094b71aff12c384f4fc60f87ff2915.
          ATTACHMENT ID: 12869503

          +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 47 warning messages.

          -1 release audit. The applied patch generated 2 release audit warnings (more than the master's current 0 warnings).

          -1 lineLengths. The patch introduces the following lines longer than 100:
          + stmt.execute(String.format("CREATE INDEX %s ON %s (LPAD(UPPER(NAME),11,'x')||'_xyz') ", indxTable, fullTableName));
          + assertExplainPlan(actualExplainPlan, schemaName, dataTableName, indxTable, isNamespaceEnabled);

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

          Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/893//testReport/
          Release audit warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/893//artifact/patchprocess/patchReleaseAuditWarnings.txt
          Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/893//artifact/patchprocess/patchJavadocWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/893//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/12869503/PHOENIX-3827_v4.patch against master branch at commit 4471b87926094b71aff12c384f4fc60f87ff2915. ATTACHMENT ID: 12869503 +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 47 warning messages. -1 release audit . The applied patch generated 2 release audit warnings (more than the master's current 0 warnings). -1 lineLengths . The patch introduces the following lines longer than 100: + stmt.execute(String.format("CREATE INDEX %s ON %s (LPAD(UPPER(NAME),11,'x')||'_xyz') ", indxTable, fullTableName)); + assertExplainPlan(actualExplainPlan, schemaName, dataTableName, indxTable, isNamespaceEnabled); +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/893//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/893//artifact/patchprocess/patchReleaseAuditWarnings.txt Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/893//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/893//console This message is automatically generated.
          Hide
          jamestaylor James Taylor added a comment -

          +1. All looks good now, Rajeshbabu Chintaguntla.

          Show
          jamestaylor James Taylor added a comment - +1. All looks good now, Rajeshbabu Chintaguntla .
          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/12869519/PHOENIX-3827_v5.patch
          against master branch at commit 4471b87926094b71aff12c384f4fc60f87ff2915.
          ATTACHMENT ID: 12869519

          +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 47 warning messages.

          -1 release audit. The applied patch generated 2 release audit warnings (more than the master's current 0 warnings).

          -1 lineLengths. The patch introduces the following lines longer than 100:
          + stmt.execute(String.format("CREATE INDEX %s ON %s (LPAD(UPPER(NAME),11,'x')||'_xyz') ", indxTable, fullTableName));
          + assertExplainPlan(actualExplainPlan, schemaName, dataTableName, indxTable, isNamespaceEnabled);

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

          Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/895//testReport/
          Release audit warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/895//artifact/patchprocess/patchReleaseAuditWarnings.txt
          Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/895//artifact/patchprocess/patchJavadocWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/895//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/12869519/PHOENIX-3827_v5.patch against master branch at commit 4471b87926094b71aff12c384f4fc60f87ff2915. ATTACHMENT ID: 12869519 +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 47 warning messages. -1 release audit . The applied patch generated 2 release audit warnings (more than the master's current 0 warnings). -1 lineLengths . The patch introduces the following lines longer than 100: + stmt.execute(String.format("CREATE INDEX %s ON %s (LPAD(UPPER(NAME),11,'x')||'_xyz') ", indxTable, fullTableName)); + assertExplainPlan(actualExplainPlan, schemaName, dataTableName, indxTable, isNamespaceEnabled); -1 core tests . The patch failed these unit tests: Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/895//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/895//artifact/patchprocess/patchReleaseAuditWarnings.txt Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/895//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/895//console This message is automatically generated.
          Hide
          jamestaylor James Taylor added a comment -

          +1 for v5, but please add comment that we don't need postPut and postDelete because Phoenix always does batch mutations.

          Show
          jamestaylor James Taylor added a comment - +1 for v5, but please add comment that we don't need postPut and postDelete because Phoenix always does batch mutations.
          Hide
          rajeshbabu Rajeshbabu Chintaguntla added a comment -

          Thanks for review James Taylor. Committed v5 to master and I can remove the hooks in other branches as well right?

          Show
          rajeshbabu Rajeshbabu Chintaguntla added a comment - Thanks for review James Taylor . Committed v5 to master and I can remove the hooks in other branches as well right?
          Hide
          jamestaylor James Taylor added a comment -

          Yes, true - we don't need those hooks in other branches.

          Show
          jamestaylor James Taylor added a comment - Yes, true - we don't need those hooks in other branches.
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Jenkins build Phoenix-master #1621 (See https://builds.apache.org/job/Phoenix-master/1621/)
          PHOENIX-3827 Make use of HBASE-15600 to write local index mutations (rajeshbabu: rev a2f4d7eebec621b58204a9eb78d552f18dcbcf24)

          • (edit) phoenix-core/src/main/java/org/apache/phoenix/hbase/index/Indexer.java
          • (edit) phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolForPartialBuildWithNamespaceEnabledIT.java
          • (edit) phoenix-core/src/it/java/org/apache/phoenix/end2end/index/MutableIndexFailureIT.java
          • (edit) phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolForPartialBuildIT.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Jenkins build Phoenix-master #1621 (See https://builds.apache.org/job/Phoenix-master/1621/ ) PHOENIX-3827 Make use of HBASE-15600 to write local index mutations (rajeshbabu: rev a2f4d7eebec621b58204a9eb78d552f18dcbcf24) (edit) phoenix-core/src/main/java/org/apache/phoenix/hbase/index/Indexer.java (edit) phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolForPartialBuildWithNamespaceEnabledIT.java (edit) phoenix-core/src/it/java/org/apache/phoenix/end2end/index/MutableIndexFailureIT.java (edit) phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolForPartialBuildIT.java
          Hide
          lhofhansl Lars Hofhansl added a comment -

          What about the other Phoenix branches?

          Show
          lhofhansl Lars Hofhansl added a comment - What about the other Phoenix branches?
          Hide
          lhofhansl Lars Hofhansl added a comment -

          NM. Only works on 1.3.0+

          Show
          lhofhansl Lars Hofhansl added a comment - NM. Only works on 1.3.0+

            People

            • Assignee:
              rajeshbabu Rajeshbabu Chintaguntla
              Reporter:
              rajeshbabu Rajeshbabu Chintaguntla
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development