Cassandra
  1. Cassandra
  2. CASSANDRA-5492

Backport row-level bloom filter removal to 1.2

    Details

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

      Description

      With the possible presence of range tombstones, it is erroneous to skip checking for a given column in SSTableNamesIterator because the bloom filter says it is not there.

        Activity

        Ryan McGuire made changes -
        Labels qa-resolved
        Sylvain Lebresne made changes -
        Status Testing [ 10012 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Sylvain Lebresne made changes -
        Fix Version/s 1.2.6 [ 12324449 ]
        Fix Version/s 1.2.5 [ 12324301 ]
        Jonathan Ellis made changes -
        Status Patch Available [ 10002 ] Testing [ 10012 ]
        Jonathan Ellis made changes -
        Status Reopened [ 4 ] Patch Available [ 10002 ]
        Jonathan Ellis made changes -
        Tester enigmacurry
        Jonathan Ellis made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Jonathan Ellis made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Jonathan Ellis added a comment -

        it did took a few minutes to convince myself that the changes around skipBloomFilter were breaking any backward compatibility

        In fairness, the confusion was caused by applying CASSANDRA-5385 to 1.2 unnecessarily; I changed it back to (a slightly simplified version of) the way it was before.

        Committed with nits addressed.

        Show
        Jonathan Ellis added a comment - it did took a few minutes to convince myself that the changes around skipBloomFilter were breaking any backward compatibility In fairness, the confusion was caused by applying CASSANDRA-5385 to 1.2 unnecessarily; I changed it back to (a slightly simplified version of) the way it was before. Committed with nits addressed.
        Hide
        Sylvain Lebresne added a comment -

        but I think the code removal is fairly straightforward

        I wouldn't go all the way to 'straighforward', it did took a few minutes to convince myself that the changes around skipBloomFilter were breaking any backward compatibility . That being said, I did convinced myself, so +1.

        Nits:

        • In SSTableWriter, when creating the RIE, maybe we could add a comment saying we discard the range tombstones because they have already been persisted during the column indexing.
        • In RIE.serialize, we could move the write of the "promoted size" outside of the 'if (rie.isIndexed())' since promotedSize() returns 0 for non-indexed entries.
        Show
        Sylvain Lebresne added a comment - but I think the code removal is fairly straightforward I wouldn't go all the way to 'straighforward', it did took a few minutes to convince myself that the changes around skipBloomFilter were breaking any backward compatibility . That being said, I did convinced myself, so +1. Nits: In SSTableWriter, when creating the RIE, maybe we could add a comment saying we discard the range tombstones because they have already been persisted during the column indexing. In RIE.serialize, we could move the write of the "promoted size" outside of the 'if (rie.isIndexed())' since promotedSize() returns 0 for non-indexed entries.
        Jonathan Ellis made changes -
        Reviewer jasobrown slebresne
        Hide
        Jonathan Ellis added a comment -

        New version on top of current 1.2 (including revert of CASSANDRA-5487) pushed to http://github.com/jbellis/cassandra/commits/5492-3.

        Show
        Jonathan Ellis added a comment - New version on top of current 1.2 (including revert of CASSANDRA-5487 ) pushed to http://github.com/jbellis/cassandra/commits/5492-3 .
        Jonathan Ellis made changes -
        Description With the possible presence of range tombstones, it is erroneous to skip checking for a given column in SSTableNamesIterator because the bloom filter says it is not there.

        This is fixed by CASSANDRA-5487, which ignores the BF, but it's a shame to leave unused bloom filters cluttering up the row cache.

        If it's too risky to do a full backport we can just substitute an AlwaysPresentFilter in RIE.create, but I think the code removal is fairly straightforward.
        With the possible presence of range tombstones, it is erroneous to skip checking for a given column in SSTableNamesIterator because the bloom filter says it is not there.
        Hide
        Jonathan Ellis added a comment -

        Pushed a formatting fix.

        Show
        Jonathan Ellis added a comment - Pushed a formatting fix.
        Jonathan Ellis made changes -
        Field Original Value New Value
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Jonathan Ellis added a comment -

        Running the test suite revealed a bug on the supercolumn path. Fixed and pushed. (Same branch, so apologies if you already grabbed it.)

        Show
        Jonathan Ellis added a comment - Running the test suite revealed a bug on the supercolumn path. Fixed and pushed. (Same branch, so apologies if you already grabbed it.)
        Hide
        Jonathan Ellis added a comment - - edited

        Pushed to http://github.com/jbellis/cassandra/commits/5492-2

        Second commit adds ScrubTest back, minus testScrubFile that appears to have relied on a bogus bloom filter, which is now skipped on the read path.

        Note that the first commit cleans up the IndexHelper cruft added in CASSANDRA-5385.

        Show
        Jonathan Ellis added a comment - - edited Pushed to http://github.com/jbellis/cassandra/commits/5492-2 Second commit adds ScrubTest back, minus testScrubFile that appears to have relied on a bogus bloom filter, which is now skipped on the read path. Note that the first commit cleans up the IndexHelper cruft added in CASSANDRA-5385 .
        Jonathan Ellis created issue -

          People

          • Assignee:
            Jonathan Ellis
            Reporter:
            Jonathan Ellis
            Reviewer:
            Sylvain Lebresne
            Tester:
            Ryan McGuire
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development