Followup for CASSANDRA-7403

Author: Sylvain Lebresne

src/java/org/apache/cassandra/db/BufferExpiringCell.java

@@ -142,6 +142,7 @@ public void validateFields(CFMetaData metadata) throws MarshalException
             throw new MarshalException("The local expiration time should not be negative");
     }
 
+    @Override
     public Cell reconcile(Cell cell)
     {
         long ts1 = timestamp(), ts2 = cell.timestamp();
@@ -150,11 +151,10 @@ public Cell reconcile(Cell cell)
         // we should prefer tombstones
         if (cell instanceof DeletedCell)
             return cell;
-        // however if we're both ExpiringCells, we should prefer the one with the longest ttl
-        // (really in preference _always_ to the value comparison)
         int c = value().compareTo(cell.value());
         if (c != 0)
             return c < 0 ? cell : this;
+        // If we have same timestamp and value, prefer the longest ttl
         if (cell instanceof ExpiringCell)
         {
             int let1 = localExpirationTime, let2 = cell.getLocalDeletionTime();

src/java/org/apache/cassandra/db/NativeExpiringCell.java

@@ -128,6 +128,7 @@ public void updateDigest(MessageDigest digest)
         FBUtilities.updateWithInt(digest, getTimeToLive());
     }
 
+    @Override
     public Cell reconcile(Cell cell)
     {
         long ts1 = timestamp(), ts2 = cell.timestamp();
@@ -136,11 +137,10 @@ public Cell reconcile(Cell cell)
         // we should prefer tombstones
         if (cell instanceof DeletedCell)
             return cell;
-        // however if we're both ExpiringCells, we should prefer the one with the longest ttl
-        // (really in preference _always_ to the value comparison)
         int c = value().compareTo(cell.value());
         if (c != 0)
             return c < 0 ? cell : this;
+        // If we have same timestamp and value, prefer the longest ttl
         if (cell instanceof ExpiringCell)
         {
             int let1 = getLocalDeletionTime(), let2 = cell.getLocalDeletionTime();

src/java/org/apache/cassandra/db/index/AbstractSimplePerColumnSecondaryIndex.java

@@ -121,11 +121,12 @@ public void insert(ByteBuffer rowKey, Cell cell, OpOrder.Group opGroup)
     }
 
     public void update(ByteBuffer rowKey, Cell oldCol, Cell col, OpOrder.Group opGroup)
-    {        
+    {
         // insert the new value before removing the old one, so we never have a period
         // where the row is invisible to both queries (the opposite seems preferable); see CASSANDRA-5540                    
         insert(rowKey, col, opGroup);
-        delete(rowKey, oldCol, opGroup);
+        if (SecondaryIndexManager.shouldCleanupOldValue(oldCol, col))
+            delete(rowKey, oldCol, opGroup);
     }
 
     public void removeIndex(ByteBuffer columnName)

src/java/org/apache/cassandra/db/index/SecondaryIndexManager.java

@@ -620,6 +620,22 @@ public boolean validate(Cell cell)
         return true;
     }
 
+    static boolean shouldCleanupOldValue(Cell oldCell, Cell newCell)
+    {
+        // If any one of name/value/timestamp are different, then we
+        // should delete from the index. If not, then we can infer that
+        // at least one of the cells is an ExpiringColumn and that the
+        // difference is in the expiry time. In this case, we don't want to
+        // delete the old value from the index as the tombstone we insert
+        // will just hide the inserted value.
+        // Completely identical cells (including expiring columns with
+        // identical ttl & localExpirationTime) will not get this far due
+        // to the oldCell.equals(newColumn) in StandardUpdater.update
+        return !oldCell.name().equals(newCell.name())
+            || !oldCell.value().equals(newCell.value())
+            || oldCell.timestamp() != newCell.timestamp();
+    }
+
     public static interface Updater
     {
         /** called when constructing the index against pre-existing data */
@@ -744,20 +760,5 @@ public void updateRowLevelIndexes()
                 ((PerRowSecondaryIndex) index).index(key.getKey(), cf);
         }
 
-        private boolean shouldCleanupOldValue(Cell oldCell, Cell newCell)
-        {
-            // If any one of name/value/timestamp are different, then we
-            // should delete from the index. If not, then we can infer that
-            // at least one of the cells is an ExpiringColumn and that the
-            // difference is in the expiry time. In this case, we don't want to
-            // delete the old value from the index as the tombstone we insert
-            // will just hide the inserted value.
-            // Completely identical cells (including expiring columns with
-            // identical ttl & localExpirationTime) will not get this far due
-            // to the oldCell.equals(newColumn) in StandardUpdater.update
-            return !oldCell.name().equals(newCell.name())
-                || !oldCell.value().equals(newCell.value())
-                || oldCell.timestamp() != newCell.timestamp();
-        }
     }
 }