Clean up isMarkedForDelete to avoid relying on the current time where possible patch by slebresne; reviewed by jbellis for CASSANDRA-3716
Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/e0aec20f Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/e0aec20f Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/e0aec20f Branch: refs/heads/trunk Commit: e0aec20f451515d23c84d1ce61666b43dd928f11 Parents: 45e1d51 Author: Jonathan Ellis <[email protected]> Authored: Wed Jan 25 15:46:48 2012 -0600 Committer: Jonathan Ellis <[email protected]> Committed: Wed Jan 25 15:58:26 2012 -0600 ---------------------------------------------------------------------- .../cassandra/db/AbstractColumnContainer.java | 2 +- src/java/org/apache/cassandra/db/Column.java | 6 ++-- .../org/apache/cassandra/db/ColumnFamilyStore.java | 7 +---- .../org/apache/cassandra/db/DeletedColumn.java | 6 ----- .../org/apache/cassandra/db/ExpiringColumn.java | 18 --------------- src/java/org/apache/cassandra/db/IColumn.java | 5 ++++ .../apache/cassandra/db/filter/QueryFilter.java | 2 +- 7 files changed, 12 insertions(+), 34 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/e0aec20f/src/java/org/apache/cassandra/db/AbstractColumnContainer.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/AbstractColumnContainer.java b/src/java/org/apache/cassandra/db/AbstractColumnContainer.java index 3baba86..a87985e 100644 --- a/src/java/org/apache/cassandra/db/AbstractColumnContainer.java +++ b/src/java/org/apache/cassandra/db/AbstractColumnContainer.java @@ -197,7 +197,7 @@ public abstract class AbstractColumnContainer implements IColumnContainer, IIter public boolean hasExpiredTombstones(int gcBefore) { - if (isMarkedForDelete() && getLocalDeletionTime() < gcBefore) + if (getLocalDeletionTime() < gcBefore) return true; for (IColumn column : columns) http://git-wip-us.apache.org/repos/asf/cassandra/blob/e0aec20f/src/java/org/apache/cassandra/db/Column.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/Column.java b/src/java/org/apache/cassandra/db/Column.java index 367cc13..c5735dd 100644 --- a/src/java/org/apache/cassandra/db/Column.java +++ b/src/java/org/apache/cassandra/db/Column.java @@ -106,7 +106,7 @@ public class Column implements IColumn public boolean isMarkedForDelete() { - return false; + return (int) (System.currentTimeMillis() / 1000) >= getLocalDeletionTime(); } public long getMarkedForDeleteAt() @@ -185,7 +185,7 @@ public class Column implements IColumn public int getLocalDeletionTime() { - throw new IllegalStateException("column is not marked for delete"); + return Integer.MAX_VALUE; } public IColumn reconcile(IColumn column) @@ -278,7 +278,7 @@ public class Column implements IColumn public boolean hasExpiredTombstones(int gcBefore) { - return isMarkedForDelete() && getLocalDeletionTime() < gcBefore; + return getLocalDeletionTime() < gcBefore; } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/e0aec20f/src/java/org/apache/cassandra/db/ColumnFamilyStore.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java index 0edae3f..15a0b64 100644 --- a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java +++ b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java @@ -750,10 +750,7 @@ public class ColumnFamilyStore implements ColumnFamilyStoreMBean // remove columns if // (a) the column itself is tombstoned or // (b) the CF is tombstoned and the column is not newer than it - // - // Note that we need the inequality below for case (a) to be strict for expiring columns - // to work correctly -- see the comment in ExpiringColumn.isMarkedForDelete(). - if ((c.isMarkedForDelete() && c.getLocalDeletionTime() < gcBefore) + if (c.getLocalDeletionTime() < gcBefore || c.timestamp() <= cf.getMarkedForDeleteAt()) { iter.remove(); @@ -779,7 +776,7 @@ public class ColumnFamilyStore implements ColumnFamilyStoreMBean // (a) the subcolumn itself is tombstoned or // (b) the supercolumn is tombstoned and the subcolumn is not newer than it if (subColumn.timestamp() <= minTimestamp - || (subColumn.isMarkedForDelete() && subColumn.getLocalDeletionTime() < gcBefore)) + || subColumn.getLocalDeletionTime() < gcBefore) { subIter.remove(); } http://git-wip-us.apache.org/repos/asf/cassandra/blob/e0aec20f/src/java/org/apache/cassandra/db/DeletedColumn.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/DeletedColumn.java b/src/java/org/apache/cassandra/db/DeletedColumn.java index eff8c5c..c1986ae 100644 --- a/src/java/org/apache/cassandra/db/DeletedColumn.java +++ b/src/java/org/apache/cassandra/db/DeletedColumn.java @@ -39,12 +39,6 @@ public class DeletedColumn extends Column } @Override - public boolean isMarkedForDelete() - { - return true; - } - - @Override public long getMarkedForDeleteAt() { return timestamp; http://git-wip-us.apache.org/repos/asf/cassandra/blob/e0aec20f/src/java/org/apache/cassandra/db/ExpiringColumn.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/ExpiringColumn.java b/src/java/org/apache/cassandra/db/ExpiringColumn.java index 1309e24..adf1913 100644 --- a/src/java/org/apache/cassandra/db/ExpiringColumn.java +++ b/src/java/org/apache/cassandra/db/ExpiringColumn.java @@ -75,24 +75,6 @@ public class ExpiringColumn extends Column } @Override - public boolean isMarkedForDelete() - { - /* - * For compaction, we need to ensure that at all time if - * localExpirationTime < gcbefore, then isMarkedForDelete() == true - * (otherwise LCR may expire columns between it's two phases compaction -- see #3579). - * - * Since during compaction we know that at all time, gcbefore <= now - * (the = is important in case where gc_grace=0), it follows that to - * ensure the propery above we need for isMarkedForDelete to be - * now > localExpirationTime (*not* now >= localExpiration). For the - * same reason, compaction should consider a column tomstoned if - * getLocalDeletionTime() < gcbefore, *not* if getLocalDeletionTime() <= gcbefore. - */ - return (int) (System.currentTimeMillis() / 1000 ) > localExpirationTime; - } - - @Override public int size() { /* http://git-wip-us.apache.org/repos/asf/cassandra/blob/e0aec20f/src/java/org/apache/cassandra/db/IColumn.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/IColumn.java b/src/java/org/apache/cassandra/db/IColumn.java index e75d069..903fa12 100644 --- a/src/java/org/apache/cassandra/db/IColumn.java +++ b/src/java/org/apache/cassandra/db/IColumn.java @@ -33,7 +33,12 @@ public interface IColumn { public static final int MAX_NAME_LENGTH = FBUtilities.MAX_UNSIGNED_SHORT; + /** + * @return true if the column has been deleted (is a tombstone). This depends on comparing the server clock + * with getLocalDeletionTime, so it can change during a single request if you're not careful. + */ public boolean isMarkedForDelete(); + public long getMarkedForDeleteAt(); public long mostRecentLiveChangeAt(); public ByteBuffer name(); http://git-wip-us.apache.org/repos/asf/cassandra/blob/e0aec20f/src/java/org/apache/cassandra/db/filter/QueryFilter.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/filter/QueryFilter.java b/src/java/org/apache/cassandra/db/filter/QueryFilter.java index 8dc0dd4..e647957 100644 --- a/src/java/org/apache/cassandra/db/filter/QueryFilter.java +++ b/src/java/org/apache/cassandra/db/filter/QueryFilter.java @@ -150,7 +150,7 @@ public class QueryFilter // and if its container is deleted, the column must be changed more recently than the container tombstone (2) // (since otherwise, the only thing repair cares about is the container tombstone) long maxChange = column.mostRecentLiveChangeAt(); - return (!column.isMarkedForDelete() || column.getLocalDeletionTime() >= gcBefore || maxChange > column.getMarkedForDeleteAt()) // (1) + return (column.getLocalDeletionTime() >= gcBefore || maxChange > column.getMarkedForDeleteAt()) // (1) && (!container.isMarkedForDelete() || maxChange > container.getMarkedForDeleteAt()); // (2) }
