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

        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.
        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 .
        Hide
        Jonathan Ellis added a comment -

        Pushed a formatting fix.

        Show
        Jonathan Ellis added a comment - Pushed a formatting fix.
        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 .

          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