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

Treat delete markers consistently with puts for point-in-time scans

    Details

    • Type: Bug
    • Status: Patch Available
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 4.13.0
    • Labels:
      None

      Description

      The IndexScrutinyTool relies on doing point-in-time scans to determine consistency between the index and data tables. Unfortunately, deletes to the tables cause a problem with this approach, since delete markers take effect even if they're at a later time stamp than the point-in-time at which the scan is being done (unless KEEP_DELETED_CELLS is true). The logic of this is that scans should get the same results before and after a compaction take place.

      Taking snapshots does not help with this since they cannot be taken at a point-in-time and the delete markers will act the same way - there's no way to guarantee that the index and data table snapshots have the same "logical" set of data.

      Using raw scans would allow us to see the delete markers and do the correct point-in-time filtering ourselves. We'd need to write the filters to do this correctly (see the Tephra TransactionVisibilityFilter for an implementation of this that could be adapted). We'd also need to hook this into Phoenix or potentially dip down to the HBase level to do this.

      Thanks for brainstorming on this with me, Lars Hofhansl.

      1. PHOENIX-4277_wip.patch
        6 kB
        James Taylor
      2. PHOENIX-4277_v3.patch
        8 kB
        James Taylor
      3. PHOENIX-4277_v2.patch
        8 kB
        James Taylor

        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/12893327/PHOENIX-4277_v3.patch
        against master branch at commit 7cdcb2313b08d2eaeb775f0c989642f8d416cfb6.
        ATTACHMENT ID: 12893327

        +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 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:
        + return new ScanInfo(scanInfo.getConfiguration(), scanInfo.getFamily(), Math.max(scanInfo.getMinVersions(), 1),
        + public KeyValueScanner preStoreScannerOpen(final ObserverContext<RegionCoprocessorEnvironment> c,
        + if (scan.isRaw() || ScanInfoUtil.isKeepDeletedCells(store.getScanInfo()) || scan.getTimeRange().getMax() == HConstants.LATEST_TIMESTAMP || TransactionUtil.isTransactionalTimestamp(scan.getTimeRange().getMax())) {

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

        Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/1564//testReport/
        Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/1564//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/12893327/PHOENIX-4277_v3.patch against master branch at commit 7cdcb2313b08d2eaeb775f0c989642f8d416cfb6. ATTACHMENT ID: 12893327 +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 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: + return new ScanInfo(scanInfo.getConfiguration(), scanInfo.getFamily(), Math.max(scanInfo.getMinVersions(), 1), + public KeyValueScanner preStoreScannerOpen(final ObserverContext<RegionCoprocessorEnvironment> c, + if (scan.isRaw() || ScanInfoUtil.isKeepDeletedCells(store.getScanInfo()) || scan.getTimeRange().getMax() == HConstants.LATEST_TIMESTAMP || TransactionUtil.isTransactionalTimestamp(scan.getTimeRange().getMax())) { -1 core tests . The patch failed these unit tests: Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/1564//testReport/ Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/1564//console This message is automatically generated.
        Hide
        jamestaylor James Taylor added a comment -

        Vincent Poon - how about adding a test for the index scrutiny tool (either the test one or the real one) to show that with this patch scrutiny works while the tables are taking writes?

        Show
        jamestaylor James Taylor added a comment - Vincent Poon - how about adding a test for the index scrutiny tool (either the test one or the real one) to show that with this patch scrutiny works while the tables are taking writes?
        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/12892721/PHOENIX-4277_v2.patch
        against master branch at commit bab06d688acde9801ae171420f94871b8e78684f.
        ATTACHMENT ID: 12892721

        +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 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:
        + return new ScanInfo(scanInfo.getConfiguration(), scanInfo.getFamily(), Math.max(scanInfo.getMinVersions(), 1),
        + public KeyValueScanner preStoreScannerOpen(final ObserverContext<RegionCoprocessorEnvironment> c,
        + if (scan.isRaw() || ScanInfoUtil.isKeepDeletedCells(store.getScanInfo()) || scan.getTimeRange().getMax() == HConstants.LATEST_TIMESTAMP || TransactionUtil.isTransactionalTimestamp(scan.getTimeRange().getMax())) {

        -1 core tests. The patch failed these unit tests:
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.SaltedViewIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.IndexExtendedIT

        Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/1550//testReport/
        Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/1550//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/12892721/PHOENIX-4277_v2.patch against master branch at commit bab06d688acde9801ae171420f94871b8e78684f. ATTACHMENT ID: 12892721 +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 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: + return new ScanInfo(scanInfo.getConfiguration(), scanInfo.getFamily(), Math.max(scanInfo.getMinVersions(), 1), + public KeyValueScanner preStoreScannerOpen(final ObserverContext<RegionCoprocessorEnvironment> c, + if (scan.isRaw() || ScanInfoUtil.isKeepDeletedCells(store.getScanInfo()) || scan.getTimeRange().getMax() == HConstants.LATEST_TIMESTAMP || TransactionUtil.isTransactionalTimestamp(scan.getTimeRange().getMax())) { -1 core tests . The patch failed these unit tests: ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.SaltedViewIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.IndexExtendedIT Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/1550//testReport/ Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/1550//console This message is automatically generated.
        Hide
        tdsilva Thomas D'Silva added a comment -

        +1

        Show
        tdsilva Thomas D'Silva added a comment - +1
        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/12892717/PHOENIX-4277_v2.patch
        against master branch at commit bab06d688acde9801ae171420f94871b8e78684f.
        ATTACHMENT ID: 12892717

        +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 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:
        + return new ScanInfo(scanInfo.getConfiguration(), scanInfo.getFamily(), Math.max(scanInfo.getMinVersions(), 1),
        + public KeyValueScanner preStoreScannerOpen(final ObserverContext<RegionCoprocessorEnvironment> c,
        + if (scan.isRaw() || scan.getTimeRange().getMax() == HConstants.LATEST_TIMESTAMP || TransactionUtil.isTransactionalTimestamp(scan.getTimeRange().getMax())) {

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

        Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/1549//testReport/
        Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/1549//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/12892717/PHOENIX-4277_v2.patch against master branch at commit bab06d688acde9801ae171420f94871b8e78684f. ATTACHMENT ID: 12892717 +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 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: + return new ScanInfo(scanInfo.getConfiguration(), scanInfo.getFamily(), Math.max(scanInfo.getMinVersions(), 1), + public KeyValueScanner preStoreScannerOpen(final ObserverContext<RegionCoprocessorEnvironment> c, + if (scan.isRaw() || scan.getTimeRange().getMax() == HConstants.LATEST_TIMESTAMP || TransactionUtil.isTransactionalTimestamp(scan.getTimeRange().getMax())) { -1 core tests . The patch failed these unit tests: Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/1549//testReport/ Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/1549//console This message is automatically generated.
        Hide
        jamestaylor James Taylor added a comment -

        Please review, Thomas D'Silva. This enables point-in-time scans when there are deletes. This will allow us to scrutinize an index when the table is taking writes. We don't need to do this for scans over transactional tables.

        Show
        jamestaylor James Taylor added a comment - Please review, Thomas D'Silva . This enables point-in-time scans when there are deletes. This will allow us to scrutinize an index when the table is taking writes. We don't need to do this for scans over transactional tables.
        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/12890635/PHOENIX-4277_wip.patch
        against master branch at commit 0f4528e3dd08ffab6385f0c0f48fbd3df991978a.
        ATTACHMENT ID: 12890635

        +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 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:
        + return new ScanInfo(scanInfo.getConfiguration(), scanInfo.getFamily(), Math.max(scanInfo.getMinVersions(), 1),
        + public KeyValueScanner preStoreScannerOpen(final ObserverContext<RegionCoprocessorEnvironment> c,

        -1 core tests. The patch failed these unit tests:
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.SysTableNamespaceMappedStatsCollectorIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.IndexExtendedIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ColumnEncodedMutableNonTxStatsCollectorIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ColumnEncodedMutableTxStatsCollectorIT

        Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/1534//testReport/
        Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/1534//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/12890635/PHOENIX-4277_wip.patch against master branch at commit 0f4528e3dd08ffab6385f0c0f48fbd3df991978a. ATTACHMENT ID: 12890635 +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 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: + return new ScanInfo(scanInfo.getConfiguration(), scanInfo.getFamily(), Math.max(scanInfo.getMinVersions(), 1), + public KeyValueScanner preStoreScannerOpen(final ObserverContext<RegionCoprocessorEnvironment> c, -1 core tests . The patch failed these unit tests: ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.SysTableNamespaceMappedStatsCollectorIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.IndexExtendedIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ColumnEncodedMutableNonTxStatsCollectorIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ColumnEncodedMutableTxStatsCollectorIT Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/1534//testReport/ Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/1534//console This message is automatically generated.
        Hide
        jamestaylor James Taylor added a comment -

        Here's a quick patch, Vincent Poon. It successfully handles point-in-time deletes without needing to set KEEP_DELETED_CELLS to true on the table. If you could add a test around doing a scrutiny while deletes are occurring on a table, I think we'll be good to go.

        Show
        jamestaylor James Taylor added a comment - Here's a quick patch, Vincent Poon . It successfully handles point-in-time deletes without needing to set KEEP_DELETED_CELLS to true on the table. If you could add a test around doing a scrutiny while deletes are occurring on a table, I think we'll be good to go.
        Hide
        jamestaylor James Taylor added a comment - - edited

        Vincent Poon - after further discussion, another easier option surfaced. HBase has the ability to run a scan with KEEP_DELETED_CELLS enabled. Phoenix could leverage this for any point-in-time scan (i.e. a scan with a max time range set to a value other than LATEST_TIMESTAMP), so no need for any new scan attribute or client-side change, since the intent with point-in-time scans in Phoenix is to treat delete markers consistently with updates (i'm not sure of the logic of seeing prior update version which would disappear with compaction while not seeing deletes).

        Here's the new coprocessor we could add to BaseScannerRegionObserver:

          @Override
          public KeyValueScanner preStoreScannerOpen(final ObserverContext<RegionCoprocessorEnvironment> c,
              final Store store, final Scan scan, final NavigableSet<byte[]> targetCols,
              final KeyValueScanner s) throws IOException {
        
            if (scan.isRaw() || scan.getTimeRange().getMax() == HConstants.LATEST_TIMESTAMP) {
              return s;
            }
        
            ScanInfo oldScanInfo = store.getScanInfo();
        
            ScanInfo scanInfo =
                new ScanInfo(oldScanInfo.getFamily(), Math.max(oldScanInfo.getMinVersions(), 1),
                    oldScanInfo.getMaxVersions(), oldScanInfo.getTtl(), KeepDeletedCells.TRUE,
                    oldScanInfo.getTimeToPurgeDeletes(), oldScanInfo.getComparator());
            return new StoreScanner(store, scanInfo, scan, targetCols,
                c.getEnvironment().getRegion().getReadpoint(scan.getIsolationLevel()));
          }
        

        This will prevent us from having to replicate in Phoenix the filtering HBase does for deletes in a user-level Filter (which the HBase community tried, but failed to do already).

        We'd need to investigate how to do this for snapshot reads, but the above should fix the current issue with the IndexScrutinyTool.

        Show
        jamestaylor James Taylor added a comment - - edited Vincent Poon - after further discussion, another easier option surfaced. HBase has the ability to run a scan with KEEP_DELETED_CELLS enabled. Phoenix could leverage this for any point-in-time scan (i.e. a scan with a max time range set to a value other than LATEST_TIMESTAMP), so no need for any new scan attribute or client-side change, since the intent with point-in-time scans in Phoenix is to treat delete markers consistently with updates (i'm not sure of the logic of seeing prior update version which would disappear with compaction while not seeing deletes). Here's the new coprocessor we could add to BaseScannerRegionObserver: @Override public KeyValueScanner preStoreScannerOpen( final ObserverContext<RegionCoprocessorEnvironment> c, final Store store, final Scan scan, final NavigableSet< byte []> targetCols, final KeyValueScanner s) throws IOException { if (scan.isRaw() || scan.getTimeRange().getMax() == HConstants.LATEST_TIMESTAMP) { return s; } ScanInfo oldScanInfo = store.getScanInfo(); ScanInfo scanInfo = new ScanInfo(oldScanInfo.getFamily(), Math .max(oldScanInfo.getMinVersions(), 1), oldScanInfo.getMaxVersions(), oldScanInfo.getTtl(), KeepDeletedCells.TRUE, oldScanInfo.getTimeToPurgeDeletes(), oldScanInfo.getComparator()); return new StoreScanner(store, scanInfo, scan, targetCols, c.getEnvironment().getRegion().getReadpoint(scan.getIsolationLevel())); } This will prevent us from having to replicate in Phoenix the filtering HBase does for deletes in a user-level Filter (which the HBase community tried, but failed to do already). We'd need to investigate how to do this for snapshot reads, but the above should fix the current issue with the IndexScrutinyTool.

          People

          • Assignee:
            jamestaylor James Taylor
            Reporter:
            jamestaylor James Taylor
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:

              Development