Uploaded image for project: 'Cassandra'
  1. Cassandra
  2. CASSANDRA-10266

Introduce direct unit test coverage for Rows

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Fix Version/s: 3.0.0 rc1
    • Component/s: None
    • Labels:
      None

      Description

      As with much of the codebase, we have no direct unit test coverage for Rows, and we should remedy this given how central it is to behaviour.

        Issue Links

          Activity

          Hide
          bdeggleston Blake Eggleston added a comment -

          The linked branch adds unit tests for Rows, as well as a test for Cells.reconcileComplex, which wasn't tested previously.

          I found a bug in Rows.diff where complex deletion info would be discarded if both rows had column data for the same collection column. This created a DataResolver bug for collection columns that could cause multiple overwrites to merge together after a read repair

          For instance, on a 3 node cluster, with the table CREATE TABLE numbers (k INT PRIMARY KEY, m MAP<INT, INT>);, and updates:

          UPDATE numbers SET m={0:0} WHERE k=0; // is received by all nodes
          UPDATE numbers SET m={1:1} WHERE k=0; // is only received by node A
          

          a quorum read including node A would initially return 1:1 for m. However, a read repair would only send the cell for 1:1, not the complex delete, so subsequent reads not including node A would return 0:0, 1:1.

          I've fixed the bug here, and added a few DataResolver tests that test collection columns.

          Show
          bdeggleston Blake Eggleston added a comment - The linked branch adds unit tests for Rows , as well as a test for Cells.reconcileComplex , which wasn't tested previously. I found a bug in Rows.diff where complex deletion info would be discarded if both rows had column data for the same collection column. This created a DataResolver bug for collection columns that could cause multiple overwrites to merge together after a read repair For instance, on a 3 node cluster, with the table CREATE TABLE numbers (k INT PRIMARY KEY, m MAP<INT, INT>); , and updates: UPDATE numbers SET m={0:0} WHERE k=0; // is received by all nodes UPDATE numbers SET m={1:1} WHERE k=0; // is only received by node A a quorum read including node A would initially return 1:1 for m. However, a read repair would only send the cell for 1:1 , not the complex delete, so subsequent reads not including node A would return 0:0, 1:1 . I've fixed the bug here , and added a few DataResolver tests that test collection columns.
          Hide
          jbellis Jonathan Ellis added a comment -

          Nice work!

          Show
          jbellis Jonathan Ellis added a comment - Nice work!
          Hide
          benedict Benedict added a comment -

          Is there any chance we could introduce some randomness to this test? I know most of the codebase takes this approach of defining a specific test, but I'd like to start pushing us in the direction of randomised testing as standard, especially for critical pieces of infrastructure like this. Are you confident that all possible combinations are explored? Ironically, the fact that you caught a bug (which is fantastic, btw) makes me more inclined to request this, as it looks like it was dependent on selecting a specific relationship, and there may be others we do not think to select.

          We could instead generate two (or more) random Rows, and confirm that all of the differences are reported (and no more), and just run this many times (or a handful, and have a LongTest equivalent that just runs the same for much longer). It would also mean the only difference between each of the testEmpty* etc are just configuration, so make the tests easier to modify. This could most likely be reused again for any testing of partition-wide behaviours. WDYT?

          Show
          benedict Benedict added a comment - Is there any chance we could introduce some randomness to this test? I know most of the codebase takes this approach of defining a specific test, but I'd like to start pushing us in the direction of randomised testing as standard, especially for critical pieces of infrastructure like this. Are you confident that all possible combinations are explored? Ironically, the fact that you caught a bug (which is fantastic, btw) makes me more inclined to request this, as it looks like it was dependent on selecting a specific relationship, and there may be others we do not think to select. We could instead generate two (or more) random Rows, and confirm that all of the differences are reported (and no more), and just run this many times (or a handful, and have a LongTest equivalent that just runs the same for much longer). It would also mean the only difference between each of the testEmpty* etc are just configuration, so make the tests easier to modify. This could most likely be reused again for any testing of partition-wide behaviours. WDYT?
          Hide
          bdeggleston Blake Eggleston added a comment -

          I think adding random testing is a great idea. In fact, I wrote a randomized failure simulator test for epaxos (here) that was very useful.

          That said, I'm not sure that randomized testing of lower level classes like Rows is the best way to go. First, checking the result of random inputs is basically going to require rewriting each of the methods in the test class to verify their output. Second, although these methods are used all over the place, they're pretty narrow in scope, and not too difficult to exhaustively unit test.

          I think randomized testing would be most valuable when testing how multiple classes work together, ideally simulating the interaction between multiple nodes. Randomized integration tests basically. For instance, testing DataResolver with random (and repeatable) sequences of node query combinations, messaging failures, memtable flushes, and compactions would have revealed the bug found here, and it probably would have exposed bugs that existed in other classes as well. The code used to verify the results would be simpler as well.

          Show
          bdeggleston Blake Eggleston added a comment - I think adding random testing is a great idea. In fact, I wrote a randomized failure simulator test for epaxos ( here ) that was very useful. That said, I'm not sure that randomized testing of lower level classes like Rows is the best way to go. First, checking the result of random inputs is basically going to require rewriting each of the methods in the test class to verify their output. Second, although these methods are used all over the place, they're pretty narrow in scope, and not too difficult to exhaustively unit test. I think randomized testing would be most valuable when testing how multiple classes work together, ideally simulating the interaction between multiple nodes. Randomized integration tests basically. For instance, testing DataResolver with random (and repeatable) sequences of node query combinations, messaging failures, memtable flushes, and compactions would have revealed the bug found here, and it probably would have exposed bugs that existed in other classes as well. The code used to verify the results would be simpler as well.
          Hide
          benedict Benedict added a comment -

          rewriting each of the methods in the test class to verify their output.

          For something so critical this wouldn't necessarily be a bad thing, so long as they are implemented orthogonally (and we can of course implement it as inefficiently as we like). However there's no need to go that far; the randomiser can work backwards, generating the expected output at the same time as the input.

          not too difficult to exhaustively unit test.

          That's why I ask "Are you confident that all possible combinations are explored?" - this code is not so simple that I can tell if this is the case by inspection, and any future changes to this code only make that harder to guarantee. However if you're comfortable this is future proof and covers all combinations, I'll try to convince myself as well.

          Show
          benedict Benedict added a comment - rewriting each of the methods in the test class to verify their output. For something so critical this wouldn't necessarily be a bad thing, so long as they are implemented orthogonally (and we can of course implement it as inefficiently as we like). However there's no need to go that far; the randomiser can work backwards, generating the expected output at the same time as the input. not too difficult to exhaustively unit test. That's why I ask "Are you confident that all possible combinations are explored?" - this code is not so simple that I can tell if this is the case by inspection, and any future changes to this code only make that harder to guarantee. However if you're comfortable this is future proof and covers all combinations, I'll try to convince myself as well.
          Hide
          bdeggleston Blake Eggleston added a comment -

          I agree with your first point. To your second, I'm confident that I've explored every branch, but not every possible combination of inputs. As far as being future proof, I'm not sure one approach will be more future proof than another, since either approach will need to be updated as changes are made. In any case, I agree that a randomized test will be more thorough. You didn't address my second point though, which is that for a similar effort, we can exercise 10x the amount of code, even more edge cases, and more potential bugs, albeit at the cost of a fully comprehensive test of Rows (randomized testing of DataResolver would exercise merge and diff, but not copy and collectStats). I think both should be implemented, but I don't think they can both be implemented for rc1. WDYT?

          Show
          bdeggleston Blake Eggleston added a comment - I agree with your first point. To your second, I'm confident that I've explored every branch, but not every possible combination of inputs. As far as being future proof, I'm not sure one approach will be more future proof than another, since either approach will need to be updated as changes are made. In any case, I agree that a randomized test will be more thorough. You didn't address my second point though, which is that for a similar effort, we can exercise 10x the amount of code, even more edge cases, and more potential bugs, albeit at the cost of a fully comprehensive test of Rows (randomized testing of DataResolver would exercise merge and diff, but not copy and collectStats). I think both should be implemented, but I don't think they can both be implemented for rc1. WDYT?
          Hide
          benedict Benedict added a comment -

          I guess I missed that crucial sentence where you sneakily suggested covering this plus more. Am I interpreting that sentence correctly as a suggestion we actually follow up with that?

          Either way, let's commit this one (especially to get the bug fix in) and target that as a follow up.

          Show
          benedict Benedict added a comment - I guess I missed that crucial sentence where you sneakily suggested covering this plus more . Am I interpreting that sentence correctly as a suggestion we actually follow up with that? Either way, let's commit this one (especially to get the bug fix in) and target that as a follow up.
          Hide
          bdeggleston Blake Eggleston added a comment -

          Am I interpreting that sentence correctly as a suggestion we actually follow up with that?

          Yes, definitely

          Show
          bdeggleston Blake Eggleston added a comment - Am I interpreting that sentence correctly as a suggestion we actually follow up with that? Yes, definitely
          Hide
          benedict Benedict added a comment -

          Great. Committed as b263af93a2899cf12ee5f35f0518460683fdac18

          Show
          benedict Benedict added a comment - Great. Committed as b263af93a2899cf12ee5f35f0518460683fdac18

            People

            • Assignee:
              bdeggleston Blake Eggleston
              Reporter:
              benedict Benedict
              Reviewer:
              Benedict
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development