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

      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.
      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) ?

        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