cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From slebre...@apache.org
Subject [01/12] cassandra git commit: Fix handling of cells for removed column when reading legacy sstables
Date Fri, 06 Oct 2017 14:19:05 GMT
Repository: cassandra
Updated Branches:
  refs/heads/cassandra-3.0 c07441251 -> 8a424cef3
  refs/heads/cassandra-3.11 710657d82 -> 3d09901b4
  refs/heads/trunk cfee3e93b -> 9663c0093


Fix handling of cells for removed column when reading legacy sstables

Patch by Sylvain Lebresne; reviewed by Robert Stupp for CASSANDRA-13939


Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo
Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/5378ba27
Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/5378ba27
Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/5378ba27

Branch: refs/heads/cassandra-3.0
Commit: 5378ba27ff66b4f39e34d613f99b0f2eddf7990d
Parents: c074412
Author: Sylvain Lebresne <sylvain@datastax.com>
Authored: Thu Oct 5 10:30:56 2017 +0200
Committer: Sylvain Lebresne <sylvain@datastax.com>
Committed: Fri Oct 6 16:12:56 2017 +0200

----------------------------------------------------------------------
 .../org/apache/cassandra/db/LegacyLayout.java   | 70 +++++++++++++-------
 .../cassandra/db/UnfilteredDeserializer.java    | 29 +++++---
 2 files changed, 67 insertions(+), 32 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/cassandra/blob/5378ba27/src/java/org/apache/cassandra/db/LegacyLayout.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/db/LegacyLayout.java b/src/java/org/apache/cassandra/db/LegacyLayout.java
index 40b9fd3..3ba96a6 100644
--- a/src/java/org/apache/cassandra/db/LegacyLayout.java
+++ b/src/java/org/apache/cassandra/db/LegacyLayout.java
@@ -649,7 +649,7 @@ public abstract class LegacyLayout
 
         boolean foundOne = false;
         LegacyAtom atom;
-        while ((atom = readLegacyAtom(metadata, in, false)) != null)
+        while ((atom = readLegacyAtomSkippingUnknownColumn(metadata,in)) != null)
         {
             if (atom.isCell())
             {
@@ -672,6 +672,23 @@ public abstract class LegacyLayout
         return foundOne ? builder.build() : Rows.EMPTY_STATIC_ROW;
     }
 
+    private static LegacyAtom readLegacyAtomSkippingUnknownColumn(CFMetaData metadata, DataInputPlus
in)
+    throws IOException
+    {
+        while (true)
+        {
+            try
+            {
+                return readLegacyAtom(metadata, in, false);
+            }
+            catch (UnknownColumnException e)
+            {
+                // Simply skip, as the method name implies.
+            }
+        }
+
+    }
+
     private static Row getNextRow(CellGrouper grouper, PeekingIterator<? extends LegacyAtom>
cells)
     {
         if (!cells.hasNext())
@@ -1020,29 +1037,36 @@ public abstract class LegacyLayout
         };
     }
 
-    public static LegacyAtom readLegacyAtom(CFMetaData metadata, DataInputPlus in, boolean
readAllAsDynamic) throws IOException
+    public static LegacyAtom readLegacyAtom(CFMetaData metadata, DataInputPlus in, boolean
readAllAsDynamic)
+    throws IOException, UnknownColumnException
     {
-        while (true)
-        {
-            ByteBuffer cellname = ByteBufferUtil.readWithShortLength(in);
-            if (!cellname.hasRemaining())
-                return null; // END_OF_ROW
-
-            try
-            {
-                int b = in.readUnsignedByte();
-                return (b & RANGE_TOMBSTONE_MASK) != 0
-                    ? readLegacyRangeTombstoneBody(metadata, in, cellname)
-                    : readLegacyCellBody(metadata, in, cellname, b, SerializationHelper.Flag.LOCAL,
readAllAsDynamic);
-            }
-            catch (UnknownColumnException e)
-            {
-                // We can get there if we read a cell for a dropped column, and ff that is
the case,
-                // then simply ignore the cell is fine. But also not that we ignore if it's
the
-                // system keyspace because for those table we actually remove columns without
registering
-                // them in the dropped columns
-                assert metadata.ksName.equals(SystemKeyspace.NAME) || metadata.getDroppedColumnDefinition(e.columnName)
!= null : e.getMessage();
-            }
+        ByteBuffer cellname = ByteBufferUtil.readWithShortLength(in);
+        if (!cellname.hasRemaining())
+            return null; // END_OF_ROW
+
+        try
+        {
+            int b = in.readUnsignedByte();
+            return (b & RANGE_TOMBSTONE_MASK) != 0
+                   ? readLegacyRangeTombstoneBody(metadata, in, cellname)
+                   : readLegacyCellBody(metadata, in, cellname, b, SerializationHelper.Flag.LOCAL,
readAllAsDynamic);
+        }
+        catch (UnknownColumnException e)
+        {
+            // We legitimately can get here in 2 cases:
+            // 1) for system tables, because we've unceremoniously removed columns (without
registering them as dropped)
+            // 2) for dropped columns.
+            // In any other case, there is a mismatch between the schema and the data, and
we complain loudly in
+            // that case. Note that if we are in a legit case of an unknown column, we want
to simply skip that cell,
+            // but we don't do this here and re-throw the exception because the calling code
sometimes has to know
+            // about this happening. This does mean code calling this method should handle
this case properly.
+            if (!metadata.ksName.equals(SystemKeyspace.NAME) && metadata.getDroppedColumnDefinition(e.columnName)
== null)
+                throw new IllegalStateException(String.format("Got cell for unknown column
%s in sstable of %s.%s: " +
+                                                              "This suggest a problem with
the schema which doesn't list " +
+                                                              "this column. Even if that
column was dropped, it should have " +
+                                                              "been listed as such", metadata.ksName,
metadata.cfName, UTF8Type.instance.compose(e.columnName)), e);
+
+            throw e;
         }
     }
 

http://git-wip-us.apache.org/repos/asf/cassandra/blob/5378ba27/src/java/org/apache/cassandra/db/UnfilteredDeserializer.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/db/UnfilteredDeserializer.java b/src/java/org/apache/cassandra/db/UnfilteredDeserializer.java
index ea65633..0aa5741 100644
--- a/src/java/org/apache/cassandra/db/UnfilteredDeserializer.java
+++ b/src/java/org/apache/cassandra/db/UnfilteredDeserializer.java
@@ -274,16 +274,26 @@ public abstract class UnfilteredDeserializer
 
         private LegacyLayout.LegacyAtom readAtom()
         {
-            try
-            {
-                long pos = currentPosition();
-                LegacyLayout.LegacyAtom atom =  LegacyLayout.readLegacyAtom(metadata, in,
readAllAsDynamic);
-                bytesReadForNextAtom = currentPosition() - pos;
-                return atom;
-            }
-            catch (IOException e)
+            while (true)
             {
-                throw new IOError(e);
+                try
+                {
+                    long pos = currentPosition();
+                    LegacyLayout.LegacyAtom atom = LegacyLayout.readLegacyAtom(metadata,
in, readAllAsDynamic);
+                    bytesReadForNextAtom = currentPosition() - pos;
+                    return atom;
+                }
+                catch (UnknownColumnException e)
+                {
+                    // This is ok, see LegacyLayout.readLegacyAtom() for why this only happens
in case were we're ok
+                    // skipping the cell. We do want to catch this at this level however
because when that happen,
+                    // we should *not* count the byte of that discarded cell as part of the
bytes for the atom
+                    // we will eventually return, as doing so could throw the logic bytesReadForNextAtom
participates in.
+                }
+                catch (IOException e)
+                {
+                    throw new IOError(e);
+                }
             }
         }
 
@@ -407,6 +417,7 @@ public abstract class UnfilteredDeserializer
             saved = null;
             iterator.clearState();
             lastConsumedPosition = currentPosition();
+            bytesReadForNextAtom = 0;
         }
 
         // Groups atoms from the input into proper Unfiltered.


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org


Mime
View raw message