Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Fix Version/s: 2.0.1
    • Component/s: None
    • Labels:

      Description

      SSTableBoundedScanner seeks rather than scanning through rows, so it would be significantly more efficient than the existing per-key filtering that cleanup does.

        Activity

        Stu Hood created issue -
        Stu Hood made changes -
        Field Original Value New Value
        Description SSTableBoundedScanner seeks rather than scanning through rows, so it would be significantly more efficient than the existing cleanup. SSTableBoundedScanner seeks rather than scanning through rows, so it would be significantly more efficient than the existing per-key filtering that cleanup does.
        Jonathan Ellis made changes -
        Priority Major [ 3 ] Minor [ 4 ]
        Jonathan Ellis made changes -
        Labels lhf
        Stu Hood made changes -
        Assignee Stu Hood [ stuhood ]
        Hide
        Stu Hood added a comment -

        Very, very simple change to use SSTableBoundedScanner during cleanup. The bulk of the patch is test changes.

        Show
        Stu Hood added a comment - Very, very simple change to use SSTableBoundedScanner during cleanup. The bulk of the patch is test changes.
        Stu Hood made changes -
        Stu Hood made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Reviewer jbellis
        Hide
        Stu Hood added a comment -

        Oops... secondary indexes and counters actually still require a full scan to stay consistent after cleanup. 0002 Adds CleanupStrategy.

        Show
        Stu Hood added a comment - Oops... secondary indexes and counters actually still require a full scan to stay consistent after cleanup. 0002 Adds CleanupStrategy.
        Stu Hood made changes -
        Hide
        Sylvain Lebresne added a comment -

        This looks good to me, though one thing that goes away with that is that for cf that use the BoundedScanner, we don't cleanup the row cache. This is not a really big deal, but it's perfectly possible that following the cleanup, rows that are still on the node get evicted from cache before rows that are not in the ranges of the node, so it's a slight regression. Maybe we could simply add a cache cleanup that iterate through the cache and only keeps keys in range? Though one may argue it's on the fringe of this ticket, I'd prefer we add this here so we commit something that is an improvement, rather than an improvement with a slight regression to be fixed later.

        Show
        Sylvain Lebresne added a comment - This looks good to me, though one thing that goes away with that is that for cf that use the BoundedScanner, we don't cleanup the row cache. This is not a really big deal, but it's perfectly possible that following the cleanup, rows that are still on the node get evicted from cache before rows that are not in the ranges of the node, so it's a slight regression. Maybe we could simply add a cache cleanup that iterate through the cache and only keeps keys in range? Though one may argue it's on the fringe of this ticket, I'd prefer we add this here so we commit something that is an improvement, rather than an improvement with a slight regression to be fixed later.
        Jonathan Ellis made changes -
        Status Patch Available [ 10002 ] In Progress [ 3 ]
        Reviewer jbellis slebresne
        Jonathan Ellis made changes -
        Assignee Stu Hood [ stuhood ]
        Fix Version/s 2.0 [ 12322954 ]
        Gavin made changes -
        Workflow no-reopen-closed, patch-avail [ 12611099 ] patch-available, re-open possible [ 12751714 ]
        Gavin made changes -
        Workflow patch-available, re-open possible [ 12751714 ] reopen-resolved, no closed status, patch-avail, testing [ 12755032 ]
        Hide
        Jonathan Ellis added a comment -

        (SSTBS has been replaced by a SSTS constructor taking a range)

        Show
        Jonathan Ellis added a comment - (SSTBS has been replaced by a SSTS constructor taking a range)
        Jonathan Ellis made changes -
        Assignee Marcus Eriksson [ krummas ]
        Hide
        Marcus Eriksson added a comment -

        mostly an update to the last patch by Stu Hood and adding a background task to clean up the row cache

        Show
        Marcus Eriksson added a comment - mostly an update to the last patch by Stu Hood and adding a background task to clean up the row cache
        Marcus Eriksson made changes -
        Attachment 0001-CASSANDRA-2524-use-SSTableBoundedScanner-for-cleanup.patch [ 12579139 ]
        Marcus Eriksson made changes -
        Attachment 0001-CASSANDRA-2524-use-SSTableBoundedScanner-for-cleanup.patch [ 12579139 ]
        Hide
        Marcus Eriksson added a comment -

        oops, db.RowMutation should not be in the patch

        Show
        Marcus Eriksson added a comment - oops, db.RowMutation should not be in the patch
        Marcus Eriksson made changes -
        Marcus Eriksson made changes -
        Status In Progress [ 3 ] Patch Available [ 10002 ]
        Hide
        Jonathan Ellis added a comment -

        Hmm, I definitely meant to comment on this earlier... I think this gets obsoleted by CASSANDRA-5051?

        Show
        Jonathan Ellis added a comment - Hmm, I definitely meant to comment on this earlier... I think this gets obsoleted by CASSANDRA-5051 ?
        Hide
        Marcus Eriksson added a comment -

        hmm i dont see it replacing the scanner in doCleanupCompaction, so this would still be valid right?

        Show
        Marcus Eriksson added a comment - hmm i dont see it replacing the scanner in doCleanupCompaction, so this would still be valid right?
        Hide
        Marcus Eriksson added a comment -

        Vijay what do you think?

        Show
        Marcus Eriksson added a comment - Vijay what do you think?
        Hide
        Vijay added a comment -

        Marcus, I think jonathan is talking about the use of this optimization, since users might not need to run cleanup after 5051.

        Show
        Vijay added a comment - Marcus, I think jonathan is talking about the use of this optimization, since users might not need to run cleanup after 5051.
        Hide
        Marcus Eriksson added a comment -

        hmm in that case the explicit cleanup should perhaps be removed? Have to say I do like having the option to reclaim space directly, not having to wait for compaction (and when i do cleanup, it would be nice if it was quick )

        Show
        Marcus Eriksson added a comment - hmm in that case the explicit cleanup should perhaps be removed? Have to say I do like having the option to reclaim space directly, not having to wait for compaction (and when i do cleanup, it would be nice if it was quick )
        Hide
        Marcus Eriksson added a comment - - edited

        Jonathan Ellis this should definetly be deprecated after CASSANDRA-4180 though, right? (SSTBS being gone and all)

        Show
        Marcus Eriksson added a comment - - edited Jonathan Ellis this should definetly be deprecated after CASSANDRA-4180 though, right? (SSTBS being gone and all)
        Hide
        Jonathan Ellis added a comment -

        I think the analog would be to create a SSTS with a Range.

        Show
        Jonathan Ellis added a comment - I think the analog would be to create a SSTS with a Range.
        Jonathan Ellis made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Jonathan Ellis made changes -
        Fix Version/s 2.0 [ 12324629 ]
        Fix Version/s 2.0 beta 1 [ 12322954 ]
        Jonathan Ellis made changes -
        Assignee Marcus Eriksson [ krummas ] Tyler Hobbs [ thobbs ]
        Fix Version/s 2.0.1 [ 12324542 ]
        Fix Version/s 2.0 [ 12324629 ]
        Reviewer slebresne jbellis
        Hide
        Jonathan Ellis added a comment -

        Assigning to Tyler since his work on CASSANDRA-5722 gets us most of the way there.

        Show
        Jonathan Ellis added a comment - Assigning to Tyler since his work on CASSANDRA-5722 gets us most of the way there.
        Tyler Hobbs made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Hide
        Tyler Hobbs added a comment -

        Patch 2524-v1.txt (and branch) builds on Marcus's work and adds multiple range support to SSTableScanner. There didn't end up being much overlap with the work on CASSANDRA-5722.

        Show
        Tyler Hobbs added a comment - Patch 2524-v1.txt (and branch ) builds on Marcus's work and adds multiple range support to SSTableScanner. There didn't end up being much overlap with the work on CASSANDRA-5722 .
        Tyler Hobbs made changes -
        Attachment 2524-v1.txt [ 12595227 ]
        Tyler Hobbs made changes -
        Status In Progress [ 3 ] Patch Available [ 10002 ]
        Hide
        Jonathan Ellis added a comment -

        With a range-aware scanner, do we still need 5722?

        Show
        Jonathan Ellis added a comment - With a range-aware scanner, do we still need 5722?
        Hide
        Tyler Hobbs added a comment -

        With a range-aware scanner, do we still need 5722?

        When there are counters and secondary indexes, a full scan would still need to be performed, so 5722 would let us avoid that case when it's unnecessary.

        Show
        Tyler Hobbs added a comment - With a range-aware scanner, do we still need 5722? When there are counters and secondary indexes, a full scan would still need to be performed, so 5722 would let us avoid that case when it's unnecessary.
        Jonathan Ellis made changes -
        Reviewer jbellis krummas
        Hide
        Marcus Eriksson added a comment -

        looks good to me! i'll get it committed tonight

        Show
        Marcus Eriksson added a comment - looks good to me! i'll get it committed tonight
        Hide
        Marcus Eriksson added a comment -

        committed as 9e846d9ff69f825f6200f7f75fdfc53926bfc255

        Show
        Marcus Eriksson added a comment - committed as 9e846d9ff69f825f6200f7f75fdfc53926bfc255
        Marcus Eriksson made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Aleksey Yeschenko added a comment -

        This patch broke at the very least sstable2json for 2i sstables (w/ LocalPartitioner).

        Show
        Aleksey Yeschenko added a comment - This patch broke at the very least sstable2json for 2i sstables (w/ LocalPartitioner).
        Jonathan Ellis made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Hide
        Jonathan Ellis added a comment -

        patch to respect per-sstable partitioner

        Show
        Jonathan Ellis added a comment - patch to respect per-sstable partitioner
        Jonathan Ellis made changes -
        Attachment 2524-use-sstable-partitioner.txt [ 12600029 ]
        Jonathan Ellis made changes -
        Status Reopened [ 4 ] Patch Available [ 10002 ]
        Hide
        Aleksey Yeschenko added a comment -
        Show
        Aleksey Yeschenko added a comment - Jonathan Ellis +1
        Hide
        Jonathan Ellis added a comment -

        committed

        Show
        Jonathan Ellis added a comment - committed
        Jonathan Ellis made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Patch Available Patch Available
        176d 3h 55m 1 Stu Hood 14/Oct/11 05:07
        Patch Available Patch Available In Progress In Progress
        103d 21h 11m 1 Jonathan Ellis 26/Jan/12 01:19
        Patch Available Patch Available Open Open
        33d 12h 37m 1 Jonathan Ellis 21/May/13 04:59
        Open Open In Progress In Progress
        58d 18h 50m 1 Tyler Hobbs 18/Jul/13 23:49
        In Progress In Progress Patch Available Patch Available
        460d 9h 15m 2 Tyler Hobbs 31/Jul/13 19:03
        Resolved Resolved Reopened Reopened
        12d 15h 9m 1 Jonathan Ellis 26/Aug/13 22:56
        Reopened Reopened Patch Available Patch Available
        1m 7s 1 Jonathan Ellis 26/Aug/13 22:57
        Patch Available Patch Available Resolved Resolved
        13d 16h 54m 2 Jonathan Ellis 27/Aug/13 03:08

          People

          • Assignee:
            Tyler Hobbs
            Reporter:
            Stu Hood
            Reviewer:
            Marcus Eriksson
          • Votes:
            0 Vote for this issue
            Watchers:
            10 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development