Cassandra
  1. Cassandra
  2. CASSANDRA-3449

Missing null check in digest retry part of read path

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Fix Version/s: 1.0.2
    • Component/s: Core
    • Labels:
      None
    1. 3449.txt
      1 kB
      Jonathan Ellis

      Activity

      Jonathan Ellis created issue -
      Jonathan Ellis made changes -
      Field Original Value New Value
      Attachment 3449.txt [ 12502092 ]
      Jonathan Ellis made changes -
      Status Open [ 1 ] Patch Available [ 10002 ]
      Reviewer slebresne
      Hide
      Sylvain Lebresne added a comment -

      I think it is possible for the row to be null but still being in a short read situation. Like if you resolve 2 nodes whose responses look like:

        node1:  1  del  3
        node2: del  2  del
      

      and you've asked for two columns. The merging results in a null row (assuming all del have priority), but it's possible that node1 had a lot of perfectly good value after that 3, so we should retry.

      I believe the right fix could be to just have a if (row != null) before the rows.add(row) ?

      Show
      Sylvain Lebresne added a comment - I think it is possible for the row to be null but still being in a short read situation. Like if you resolve 2 nodes whose responses look like: node1: 1 del 3 node2: del 2 del and you've asked for two columns. The merging results in a null row (assuming all del have priority), but it's possible that node1 had a lot of perfectly good value after that 3, so we should retry. I believe the right fix could be to just have a if (row != null) before the rows.add(row) ?
      Hide
      Sylvain Lebresne added a comment -

      Ok, I went ahead and while committing CASSANDRA-3303 I added a null check just before the rows.add(row) line. My rational for ninja-shoving it was that this can't introduce a bug. Worst case scenario, we shouldn't do the check for short read code when the row is null in the first place and my added null check is useless. But as said above I believe we do need to do the check for short read in this case, so I'm closing this. But feel free to reopen if you spot a mistake in my reasoning.

      Show
      Sylvain Lebresne added a comment - Ok, I went ahead and while committing CASSANDRA-3303 I added a null check just before the rows.add(row) line. My rational for ninja-shoving it was that this can't introduce a bug. Worst case scenario, we shouldn't do the check for short read code when the row is null in the first place and my added null check is useless. But as said above I believe we do need to do the check for short read in this case, so I'm closing this. But feel free to reopen if you spot a mistake in my reasoning.
      Sylvain Lebresne made changes -
      Status Patch Available [ 10002 ] Resolved [ 5 ]
      Resolution Fixed [ 1 ]
      Gavin made changes -
      Workflow no-reopen-closed, patch-avail [ 12640707 ] patch-available, re-open possible [ 12749091 ]
      Gavin made changes -
      Workflow patch-available, re-open possible [ 12749091 ] reopen-resolved, no closed status, patch-avail, testing [ 12754041 ]

        People

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

          Dates

          • Created:
            Updated:
            Resolved:

            Development