cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jbel...@apache.org
Subject svn commit: r832798 - in /incubator/cassandra/trunk: src/java/org/apache/cassandra/db/ src/java/org/apache/cassandra/db/filter/ test/unit/org/apache/cassandra/db/
Date Wed, 04 Nov 2009 17:07:49 GMT
Author: jbellis
Date: Wed Nov  4 17:07:49 2009
New Revision: 832798

URL: http://svn.apache.org/viewvc?rev=832798&view=rev
Log:
return clones of supercolumns from memtable so caller can't accidentally mutate them, fixing
the failing test.

convert removeDeleted on SC to remove-oriented instead of clone-then-add-back to make this
hurt performance less.

patch by jbellis; reviewed by gdusbabek for CASSANDRA-510

Modified:
    incubator/cassandra/trunk/src/java/org/apache/cassandra/db/ColumnFamilyStore.java
    incubator/cassandra/trunk/src/java/org/apache/cassandra/db/Memtable.java
    incubator/cassandra/trunk/src/java/org/apache/cassandra/db/SuperColumn.java
    incubator/cassandra/trunk/src/java/org/apache/cassandra/db/filter/SliceQueryFilter.java
    incubator/cassandra/trunk/test/unit/org/apache/cassandra/db/RemoveSuperColumnTest.java

Modified: incubator/cassandra/trunk/src/java/org/apache/cassandra/db/ColumnFamilyStore.java
URL: http://svn.apache.org/viewvc/incubator/cassandra/trunk/src/java/org/apache/cassandra/db/ColumnFamilyStore.java?rev=832798&r1=832797&r2=832798&view=diff
==============================================================================
--- incubator/cassandra/trunk/src/java/org/apache/cassandra/db/ColumnFamilyStore.java (original)
+++ incubator/cassandra/trunk/src/java/org/apache/cassandra/db/ColumnFamilyStore.java Wed
Nov  4 17:07:49 2009
@@ -499,45 +499,55 @@
             return null;
         }
 
+        if (cf.isSuper())
+            removeDeletedSuper(cf, gcBefore);
+        else
+            removeDeletedStandard(cf, gcBefore);
+
         // in case of a timestamp tie, tombstones get priority over non-tombstones.
         // (we want this to be deterministic to avoid confusion.)
+        if (cf.getColumnCount() == 0 && cf.getLocalDeletionTime() <= gcBefore)
+        {
+            return null;
+        }
+        return cf;
+    }
+
+    private static void removeDeletedStandard(ColumnFamily cf, int gcBefore)
+    {
         for (byte[] cname : cf.getColumnNames())
         {
             IColumn c = cf.getColumnsMap().get(cname);
-            if (c instanceof SuperColumn)
+            if ((c.isMarkedForDelete() && c.getLocalDeletionTime() <= gcBefore)
+                || c.timestamp() <= cf.getMarkedForDeleteAt())
             {
-                long minTimestamp = Math.max(c.getMarkedForDeleteAt(), cf.getMarkedForDeleteAt());
-                // don't operate directly on the supercolumn, it could be the one in the
memtable.
-                // instead, create a new SC and add in the subcolumns that qualify.
                 cf.remove(cname);
-                SuperColumn sc = ((SuperColumn)c).cloneMeShallow();
-                for (IColumn subColumn : c.getSubColumns())
-                {
-                    if (subColumn.timestamp() > minTimestamp)
-                    {
-                        if (!subColumn.isMarkedForDelete() || subColumn.getLocalDeletionTime()
> gcBefore)
-                        {
-                            sc.addColumn(subColumn);
-                        }
-                    }
-                }
-                if (sc.getSubColumns().size() > 0 || sc.getLocalDeletionTime() > gcBefore)
+            }
+        }
+    }
+
+    private static void removeDeletedSuper(ColumnFamily cf, int gcBefore)
+    {
+        // TODO assume deletion means "most are deleted?" and add to clone, instead of remove
from original?
+        // this could be improved by having compaction, or possibly even removeDeleted, r/m
the tombstone
+        // once gcBefore has passed, so if new stuff is added in it doesn't used the wrong
algorithm forever
+        for (byte[] cname : cf.getColumnNames())
+        {
+            IColumn c = cf.getColumnsMap().get(cname);
+            long minTimestamp = Math.max(c.getMarkedForDeleteAt(), cf.getMarkedForDeleteAt());
+            for (IColumn subColumn : c.getSubColumns())
+            {
+                if (subColumn.timestamp() <= minTimestamp
+                    || (subColumn.isMarkedForDelete() && subColumn.getLocalDeletionTime()
<= gcBefore))
                 {
-                    cf.addColumn(sc);
+                    ((SuperColumn)c).remove(subColumn.name());
                 }
             }
-            else if ((c.isMarkedForDelete() && c.getLocalDeletionTime() <= gcBefore)
-                     || c.timestamp() <= cf.getMarkedForDeleteAt())
+            if (c.getSubColumns().isEmpty() && c.getLocalDeletionTime() <= gcBefore)
             {
-                cf.remove(cname);
+                cf.remove(c.name());
             }
         }
-
-        if (cf.getColumnCount() == 0 && cf.getLocalDeletionTime() <= gcBefore)
-        {
-            return null;
-        }
-        return cf;
     }
 
     /*

Modified: incubator/cassandra/trunk/src/java/org/apache/cassandra/db/Memtable.java
URL: http://svn.apache.org/viewvc/incubator/cassandra/trunk/src/java/org/apache/cassandra/db/Memtable.java?rev=832798&r1=832797&r2=832798&view=diff
==============================================================================
--- incubator/cassandra/trunk/src/java/org/apache/cassandra/db/Memtable.java (original)
+++ incubator/cassandra/trunk/src/java/org/apache/cassandra/db/Memtable.java Wed Nov  4 17:07:49
2009
@@ -262,7 +262,8 @@
         if (filter.reversed)
             ArrayUtils.reverse(columns);
         IColumn startIColumn;
-        if (DatabaseDescriptor.getColumnFamilyType(table_, filter.getColumnFamilyName()).equals("Standard"))
+        final boolean isStandard = DatabaseDescriptor.getColumnFamilyType(table_, filter.getColumnFamilyName()).equals("Standard");
+        if (isStandard)
             startIColumn = new Column(filter.start);
         else
             startIColumn = new SuperColumn(filter.start, null); // ok to not have subcolumnComparator
since we won't be adding columns to this object
@@ -298,7 +299,8 @@
 
             public IColumn next()
             {
-                return columns[curIndex_++];
+                // clone supercolumns so caller can freely removeDeleted or otherwise mutate
it
+                return isStandard ? columns[curIndex_++] : ((SuperColumn)columns[curIndex_++]).cloneMe();
             }
         };
     }
@@ -307,6 +309,7 @@
     {
         final ColumnFamily cf = columnFamilies_.get(partitioner_.decorateKey(filter.key));
         final ColumnFamily columnFamily = cf == null ? ColumnFamily.create(table_, filter.getColumnFamilyName())
: cf.cloneMeShallow();
+        final boolean isStandard = DatabaseDescriptor.getColumnFamilyType(table_, filter.getColumnFamilyName()).equals("Standard");
 
         return new SimpleAbstractColumnIterator()
         {
@@ -329,7 +332,8 @@
                     current = iter.next();
                     IColumn column = cf.getColumn(current);
                     if (column != null)
-                        return column;
+                        // clone supercolumns so caller can freely removeDeleted or otherwise
mutate it
+                        return isStandard ? column : ((SuperColumn)column).cloneMe();
                 }
                 return endOfData();
             }

Modified: incubator/cassandra/trunk/src/java/org/apache/cassandra/db/SuperColumn.java
URL: http://svn.apache.org/viewvc/incubator/cassandra/trunk/src/java/org/apache/cassandra/db/SuperColumn.java?rev=832798&r1=832797&r2=832798&view=diff
==============================================================================
--- incubator/cassandra/trunk/src/java/org/apache/cassandra/db/SuperColumn.java (original)
+++ incubator/cassandra/trunk/src/java/org/apache/cassandra/db/SuperColumn.java Wed Nov  4
17:07:49 2009
@@ -49,10 +49,15 @@
 
     SuperColumn(byte[] name, AbstractType comparator)
     {
+        this(name, new ConcurrentSkipListMap<byte[], IColumn>(comparator));
+    }
+
+    private SuperColumn(byte[] name, ConcurrentSkipListMap<byte[], IColumn> columns)
+    {
         assert name != null;
         assert name.length <= IColumn.MAX_NAME_LENGTH;
     	name_ = name;
-        columns_ = new ConcurrentSkipListMap<byte[], IColumn>(comparator);
+        columns_ = columns;
     }
 
     public AbstractType getComparator()
@@ -67,6 +72,14 @@
         return sc;
     }
 
+
+    public IColumn cloneMe()
+    {
+        SuperColumn sc = new SuperColumn(name_, new ConcurrentSkipListMap<byte[], IColumn>(columns_));
+        sc.markForDeleteAt(localDeletionTime, markedForDeleteAt);
+        return sc;
+    }
+
 	public boolean isMarkedForDelete()
 	{
 		return markedForDeleteAt > Long.MIN_VALUE;

Modified: incubator/cassandra/trunk/src/java/org/apache/cassandra/db/filter/SliceQueryFilter.java
URL: http://svn.apache.org/viewvc/incubator/cassandra/trunk/src/java/org/apache/cassandra/db/filter/SliceQueryFilter.java?rev=832798&r1=832797&r2=832798&view=diff
==============================================================================
--- incubator/cassandra/trunk/src/java/org/apache/cassandra/db/filter/SliceQueryFilter.java
(original)
+++ incubator/cassandra/trunk/src/java/org/apache/cassandra/db/filter/SliceQueryFilter.java
Wed Nov  4 17:07:49 2009
@@ -66,6 +66,8 @@
 
     public SuperColumn filterSuperColumn(SuperColumn superColumn, int gcBefore)
     {
+        // we clone shallow, then add, under the theory that generally we're interested in
a relatively small number of subcolumns.
+        // this may be a poor assumption.
         SuperColumn scFiltered = superColumn.cloneMeShallow();
         Iterator<IColumn> subcolumns;
         if (reversed)

Modified: incubator/cassandra/trunk/test/unit/org/apache/cassandra/db/RemoveSuperColumnTest.java
URL: http://svn.apache.org/viewvc/incubator/cassandra/trunk/test/unit/org/apache/cassandra/db/RemoveSuperColumnTest.java?rev=832798&r1=832797&r2=832798&view=diff
==============================================================================
--- incubator/cassandra/trunk/test/unit/org/apache/cassandra/db/RemoveSuperColumnTest.java
(original)
+++ incubator/cassandra/trunk/test/unit/org/apache/cassandra/db/RemoveSuperColumnTest.java
Wed Nov  4 17:07:49 2009
@@ -100,8 +100,8 @@
     {
         ColumnFamilyStore store = Table.open("Keyspace1").getColumnFamilyStore("Super1");
         ColumnFamily resolved = store.getColumnFamily(new NamesQueryFilter("key1", new QueryPath("Super1"),
"SC1".getBytes()));
-        assert resolved.getSortedColumns().iterator().next().getMarkedForDeleteAt() == 1;
-        assert resolved.getSortedColumns().iterator().next().getSubColumns().size() == 0;
+        assert resolved.getSortedColumns().iterator().next().getMarkedForDeleteAt() == 1
: resolved;
+        assert resolved.getSortedColumns().iterator().next().getSubColumns().size() == 0
: resolved;
         assertNull(ColumnFamilyStore.removeDeleted(resolved, Integer.MAX_VALUE));
         assertNull(store.getColumnFamily(new NamesQueryFilter("key1", new QueryPath("Super1"),
"SC1".getBytes()), Integer.MAX_VALUE));
         assertNull(store.getColumnFamily(new IdentityQueryFilter("key1", new QueryPath("Super1")),
Integer.MAX_VALUE));



Mime
View raw message