Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Fix Version/s: 1.2.0 beta 1
    • Component/s: API
    • Labels:

      Description

      cqlsh> create keyspace test with strategy_class = 'SimpleStrategy' and strategy_options:replication_factor = 1;
      cqlsh> use test;
      cqlsh:test> CREATE TABLE stats (
              ...   gid          blob,
              ...   period     int,
              ...   tid          blob, 
              ...   sum        int,
              ...   uniques           blob,
              ...   PRIMARY KEY(gid, period, tid)
              ... );
      cqlsh:test> describe columnfamily stats;
      
      CREATE TABLE stats (
        gid blob PRIMARY KEY
      ) WITH
        comment='' AND
        comparator='CompositeType(org.apache.cassandra.db.marshal.Int32Type,org.apache.cassandra.db.marshal.BytesType,org.apache.cassandra.db.marshal.UTF8Type)' AND
        read_repair_chance=0.100000 AND
        gc_grace_seconds=864000 AND
        default_validation=text AND
        min_compaction_threshold=4 AND
        max_compaction_threshold=32 AND
        replicate_on_write='true' AND
        compaction_strategy_class='SizeTieredCompactionStrategy' AND
        compression_parameters:sstable_compression='SnappyCompressor';
      

      You can see in the above output that the stats cf is created with the column validator set to text, but neither of the non primary key columns defined are text. It should either be setting metadata for those columns or not setting a default validator or some combination of the two.

      1. 4377-2.txt
        5 kB
        Sylvain Lebresne
      2. CASSANDRA-4377.patch
        5 kB
        Pavel Yaskevich
      3. 4377.txt
        2 kB
        Sylvain Lebresne

        Issue Links

          Activity

          Hide
          Sylvain Lebresne added a comment -

          What happens is that columns metadata for composite CQL3 columns refers to only one of the component of the column name. So internally they have a 'componentIndex' parameter which allow to handle them correctly (otherwise you don't know which comparator to use to display those column metadata). However, we decided that thrift users shouldn't have to care about this, so we don't expose those metadata to thrift at all. In other words, the CF does have metadata for 'sum' and 'uniques' internally, but they are not exposed to thrift.

          So I guess it is more of a cqlsh problem that shouldn't use the thrift describe call for CQL3. Instead, it should directly query the system.keyspace, system.columnfamilies and system.columns table. However, it'd be much more easier to do that with CASSANDRA-4018, but that is not in 1.1.

          Show
          Sylvain Lebresne added a comment - What happens is that columns metadata for composite CQL3 columns refers to only one of the component of the column name. So internally they have a 'componentIndex' parameter which allow to handle them correctly (otherwise you don't know which comparator to use to display those column metadata). However, we decided that thrift users shouldn't have to care about this, so we don't expose those metadata to thrift at all. In other words, the CF does have metadata for 'sum' and 'uniques' internally, but they are not exposed to thrift. So I guess it is more of a cqlsh problem that shouldn't use the thrift describe call for CQL3. Instead, it should directly query the system.keyspace, system.columnfamilies and system.columns table. However, it'd be much more easier to do that with CASSANDRA-4018 , but that is not in 1.1.
          Hide
          Nick Bailey added a comment -

          So I guess it is more of a cqlsh problem that shouldn't use the thrift describe call for CQL3.

          The main problem here from my perspective is that it was impossible to insert data into the column family except with cqlsh. Using a basic thrift batch mutate failed on the validation step because it tried to validate the value in the sum column as text (default_validation_class).

          Show
          Nick Bailey added a comment - So I guess it is more of a cqlsh problem that shouldn't use the thrift describe call for CQL3. The main problem here from my perspective is that it was impossible to insert data into the column family except with cqlsh. Using a basic thrift batch mutate failed on the validation step because it tried to validate the value in the sum column as text (default_validation_class).
          Hide
          Hussein Baghdadi added a comment - - edited

          True, I'm unable to save any data to Cassandra.
          Trying to save with Hector (uses Mutators):
          #<HInvalidRequestException me.prettyprint.hector.api.exceptions.HInvalidRequestException: InvalidRequestException(why:(String didn't validate.) [mks][stats][1234:7461726765742d3131:sum] failed validation)>

          Show
          Hussein Baghdadi added a comment - - edited True, I'm unable to save any data to Cassandra. Trying to save with Hector (uses Mutators): #<HInvalidRequestException me.prettyprint.hector.api.exceptions.HInvalidRequestException: InvalidRequestException(why:(String didn't validate.) [mks] [stats] [1234:7461726765742d3131:sum] failed validation)>
          Hide
          Sylvain Lebresne added a comment -

          it was impossible to insert data into the column family except with cqlsh. Using a basic thrift batch mutate failed on the validation step because it tried to validate the value

          Alright, that part is not expected and is likely a bug.

          Though I note that even if we fix that, since we don't expose on the thrift side everything needed to interpret the value correctly, thrift client won't be able to work with those kind of table very well. In particular they won't know how to interpret the value of a get. So I guess we need to either say clearly that CQL3 table are not meant to be accessed from thrift, or we should probably start exposing all the column metatada on the thrift side (but we need to expose the componentIndex part of the metadata in particular and the discussion on CASSANDRA-4093 is relevant for that).

          Show
          Sylvain Lebresne added a comment - it was impossible to insert data into the column family except with cqlsh. Using a basic thrift batch mutate failed on the validation step because it tried to validate the value Alright, that part is not expected and is likely a bug. Though I note that even if we fix that, since we don't expose on the thrift side everything needed to interpret the value correctly, thrift client won't be able to work with those kind of table very well. In particular they won't know how to interpret the value of a get. So I guess we need to either say clearly that CQL3 table are not meant to be accessed from thrift, or we should probably start exposing all the column metatada on the thrift side (but we need to expose the componentIndex part of the metadata in particular and the discussion on CASSANDRA-4093 is relevant for that).
          Hide
          Sylvain Lebresne added a comment -

          Attaching simple patch to allow the insertion in the case above. Truth is, I'm not really satisfied (though I don't have a clearly better option) by such a patch for 2 reasons:

          1. it will break the case for thrift where people were using compositeType and column_metadata on them. That might be 0 people we're talking about but it's still a bit annoying. An alternative here would be that for each composite column, we iterater over all column_metadata and check where one apply. This would work but this feels butt ugly.
          2. it feels to me we supporting either not enough or too much in the thrift side. Even if we support this, we still don't expose the CQL3 metadata to thrift, so one has to know the CQL schema definition to be able to create the column in the first place (or deserialize it on read). In particular, most advanced thrift client will likely still break at one point or another.

          Overall I see two reasonable approaches:

          • Either we start exposing enough on the thirft side so that thrift client can work correctly with CQL3. While this may be doable reasonably easily now, this will be increasingly difficult with things like CASSANDRA-4179, CASSANDRA-3647, ... Besides, even if we make it possible to work with CQL3 table, it doesn't mean it will be convenient since thrift won't do the grouping of columns in sparse table.
          • Be clear that you cannot work with CQL3 created table from thrift.

          But imo the in-the-middle approach that this patch would start takes the risk of polluting the thrift side without adding much.

          Show
          Sylvain Lebresne added a comment - Attaching simple patch to allow the insertion in the case above. Truth is, I'm not really satisfied (though I don't have a clearly better option) by such a patch for 2 reasons: it will break the case for thrift where people were using compositeType and column_metadata on them. That might be 0 people we're talking about but it's still a bit annoying. An alternative here would be that for each composite column, we iterater over all column_metadata and check where one apply. This would work but this feels butt ugly. it feels to me we supporting either not enough or too much in the thrift side. Even if we support this, we still don't expose the CQL3 metadata to thrift, so one has to know the CQL schema definition to be able to create the column in the first place (or deserialize it on read). In particular, most advanced thrift client will likely still break at one point or another. Overall I see two reasonable approaches: Either we start exposing enough on the thirft side so that thrift client can work correctly with CQL3. While this may be doable reasonably easily now, this will be increasingly difficult with things like CASSANDRA-4179 , CASSANDRA-3647 , ... Besides, even if we make it possible to work with CQL3 table, it doesn't mean it will be convenient since thrift won't do the grouping of columns in sparse table. Be clear that you cannot work with CQL3 created table from thrift. But imo the in-the-middle approach that this patch would start takes the risk of polluting the thrift side without adding much.
          Hide
          Sylvain Lebresne added a comment -

          To be clear, if this issue is for instance a blocker for map-reduce for CQL3, I'm not against committing it, but on a more long term/general level, I do want to note that accessing CQL3 tables to thrift is imho a larger problem and we should be clear on what we want to guarantee.

          Show
          Sylvain Lebresne added a comment - To be clear, if this issue is for instance a blocker for map-reduce for CQL3, I'm not against committing it, but on a more long term/general level, I do want to note that accessing CQL3 tables to thrift is imho a larger problem and we should be clear on what we want to guarantee.
          Hide
          Nick Bailey added a comment -

          It doesn't sound like its going to be possible to make things work well in the case of accessing cql3 data from thrift.

          If thats the case I'm in favor of making that incompatibility very explicit. I would say explicitly throwing an exception when trying to access a cql3 column family from thrift would be an acceptable solution.

          The fact that it took me quite a bit of time as well as digging around in both client code and cassandra source code to figure this bug out makes me worried for other users hitting similar problems.

          Show
          Nick Bailey added a comment - It doesn't sound like its going to be possible to make things work well in the case of accessing cql3 data from thrift. If thats the case I'm in favor of making that incompatibility very explicit. I would say explicitly throwing an exception when trying to access a cql3 column family from thrift would be an acceptable solution. The fact that it took me quite a bit of time as well as digging around in both client code and cassandra source code to figure this bug out makes me worried for other users hitting similar problems.
          Hide
          paul cannon added a comment -

          I would say explicitly throwing an exception when trying to access a cql3 column family from thrift would be an acceptable solution.

          -1 on that; it's very useful to have a lower-level access method to look at the "storage engine" layer instead of the logical CQL3 rows at times.

          Show
          paul cannon added a comment - I would say explicitly throwing an exception when trying to access a cql3 column family from thrift would be an acceptable solution. -1 on that; it's very useful to have a lower-level access method to look at the "storage engine" layer instead of the logical CQL3 rows at times.
          Hide
          Nick Bailey added a comment -

          so disable write access to cql3 cfs from thrift? And leave read access with the qualification that everything will be returned as BytesType?

          The only other option I see is to say that we are going to identify and fix all bugs like this one.

          Show
          Nick Bailey added a comment - so disable write access to cql3 cfs from thrift? And leave read access with the qualification that everything will be returned as BytesType? The only other option I see is to say that we are going to identify and fix all bugs like this one.
          Hide
          Jonathan Ellis added a comment -

          disable write access to cql3 cfs from thrift? And leave read access with the qualification that everything will be returned as BytesType?

          Sounds reasonable to me.

          Show
          Jonathan Ellis added a comment - disable write access to cql3 cfs from thrift? And leave read access with the qualification that everything will be returned as BytesType? Sounds reasonable to me.
          Hide
          Sylvain Lebresne added a comment -

          disable write access to cql3 cfs from thrift? And leave read access with the qualification that everything will be returned as BytesType?

          A variation in the same spirit could be to disable access to CQL3 table by default but add a thrift debug mode (either per-connection through or globally through JMX if we don't want to add a new thrift method) that would make thrift disable every validation. That way, by default the message that CQL3 tables should be access through CQL3 would be clear but we would have low-level read/write for debugging.

          Show
          Sylvain Lebresne added a comment - disable write access to cql3 cfs from thrift? And leave read access with the qualification that everything will be returned as BytesType? A variation in the same spirit could be to disable access to CQL3 table by default but add a thrift debug mode (either per-connection through or globally through JMX if we don't want to add a new thrift method) that would make thrift disable every validation. That way, by default the message that CQL3 tables should be access through CQL3 would be clear but we would have low-level read/write for debugging.
          Hide
          paul cannon added a comment -

          disable access to CQL3 table by default but add a thrift debug mode

          +lots.

          Show
          paul cannon added a comment - disable access to CQL3 table by default but add a thrift debug mode +lots.
          Hide
          Nick Bailey added a comment -

          sgtm

          Show
          Nick Bailey added a comment - sgtm
          Hide
          Jonathan Ellis added a comment -

          +1

          (and I'd prefer JMX otherwise I foresee clients using this as an escape hatch and fubarring things up.)

          Show
          Jonathan Ellis added a comment - +1 (and I'd prefer JMX otherwise I foresee clients using this as an escape hatch and fubarring things up.)
          Hide
          paul cannon added a comment -

          So, from an outside conversation, it sounds like there may be some confusion on exactly what is being discussed here. Are we talking about disabling Thrift access entirely to columnfamilies which use named metadata with composites, or are we just talking about not supporting Thrift addressing columns inside composites by their CQL3 names?

          The second seems eminently reasonable. The former sounds crazy.

          Show
          paul cannon added a comment - So, from an outside conversation, it sounds like there may be some confusion on exactly what is being discussed here. Are we talking about disabling Thrift access entirely to columnfamilies which use named metadata with composites, or are we just talking about not supporting Thrift addressing columns inside composites by their CQL3 names? The second seems eminently reasonable. The former sounds crazy.
          Hide
          Sylvain Lebresne added a comment -

          are we just talking about not supporting Thrift addressing columns inside composites by their CQL3 names

          That is not what I was talking about, but I don't even understand how we could ever support that.

          disabling Thrift access entirely to columnfamilies which use named metadata with composites

          That is what I'm talking about (though to be precise, it would be for composites using named metadata created through CQL3). Anyway, I don't know if that's so crazy but in any case calling it crazy doesn't help solving the problem.

          Show
          Sylvain Lebresne added a comment - are we just talking about not supporting Thrift addressing columns inside composites by their CQL3 names That is not what I was talking about, but I don't even understand how we could ever support that. disabling Thrift access entirely to columnfamilies which use named metadata with composites That is what I'm talking about (though to be precise, it would be for composites using named metadata created through CQL3 ). Anyway, I don't know if that's so crazy but in any case calling it crazy doesn't help solving the problem.
          Hide
          Nick Bailey added a comment -

          So here is where I ended up on this.

          Assuming we aren't extremely interested in fixing bugs like these when the come up then I'm all for disabling thrift access by default to cql3 cfs. From what I can tell there isn't an immediately easy way to know a cf was created through cql3 at the moment but we could add that I guess.

          On the other hand if we want to just go ahead and fix these bugs when they happen, I don't see much reason to go out of our way to disable thrift access to cql3 cfs.

          Show
          Nick Bailey added a comment - So here is where I ended up on this. Assuming we aren't extremely interested in fixing bugs like these when the come up then I'm all for disabling thrift access by default to cql3 cfs. From what I can tell there isn't an immediately easy way to know a cf was created through cql3 at the moment but we could add that I guess. On the other hand if we want to just go ahead and fix these bugs when they happen, I don't see much reason to go out of our way to disable thrift access to cql3 cfs.
          Hide
          Jonathan Ellis added a comment -

          one has to know the CQL schema definition to be able to create the column in the first place (or deserialize it on read)

          Can you remind me why we don't translate PRIMARY KEY(gid, period, tid) into a comparator of CompositeType(Int32Type, BytesType, UTF8Type)? (period -> int, tid -> bytes, sparse columns -> utf8)

          Show
          Jonathan Ellis added a comment - one has to know the CQL schema definition to be able to create the column in the first place (or deserialize it on read) Can you remind me why we don't translate PRIMARY KEY(gid, period, tid) into a comparator of CompositeType(Int32Type, BytesType, UTF8Type) ? (period -> int, tid -> bytes, sparse columns -> utf8)
          Hide
          Sylvain Lebresne added a comment -

          Can you remind me why we don't translate PRIMARY KEY(gid, period, tid) into a comparator of CompositeType(Int32Type, BytesType, UTF8Type)

          We do. The problem is actually with columns that are not part of the key. Let me try to sum that up:

          • Pre-CQL3, the ColumnDefinition name was always a full column name.
          • In CQL3, the ColumnDefinition name only correspond to one of the component of the column name, i.e. to the UTF8Type component above.
          • To be able to distinguish both case internally, we've introduce the componentIndex field in ColumnDefinition. However, we decided that we didn't wanted to expose this field to thrift, and so we don't expose to thrift the ColumnDefinition from CQL3 table.

          The net result is that as far as thrift is concerned, the CQL3 tables have no columns_metadata whatsoever. It follows that thrift clients don't know what are the correct value for the last UTF8Type component, and don't know what is the type of the corresponding value (and thus cannot serialize/deserialize said value correctly).

          Show
          Sylvain Lebresne added a comment - Can you remind me why we don't translate PRIMARY KEY(gid, period, tid) into a comparator of CompositeType(Int32Type, BytesType, UTF8Type) We do. The problem is actually with columns that are not part of the key. Let me try to sum that up: Pre-CQL3, the ColumnDefinition name was always a full column name. In CQL3, the ColumnDefinition name only correspond to one of the component of the column name, i.e. to the UTF8Type component above. To be able to distinguish both case internally, we've introduce the componentIndex field in ColumnDefinition. However, we decided that we didn't wanted to expose this field to thrift, and so we don't expose to thrift the ColumnDefinition from CQL3 table. The net result is that as far as thrift is concerned, the CQL3 tables have no columns_metadata whatsoever. It follows that thrift clients don't know what are the correct value for the last UTF8Type component, and don't know what is the type of the corresponding value (and thus cannot serialize/deserialize said value correctly).
          Hide
          Jonathan Ellis added a comment - - edited

          So basically, the problem is we're trying to maintain compatibility with (non-cql3) wide-row named columns? If we're willing to break that scenario, does the problem go away?

          I still think that naming wide row columns is nonsensical, but we could add an extra layer of protection by warning on startup in 1.2 that you need to update your schema:

          • if you have named columns and non-utf8-or-bytes comparator
          • if you have named columns but metadata shows more columns than names
          Show
          Jonathan Ellis added a comment - - edited So basically, the problem is we're trying to maintain compatibility with (non-cql3) wide-row named columns? If we're willing to break that scenario, does the problem go away? I still think that naming wide row columns is nonsensical, but we could add an extra layer of protection by warning on startup in 1.2 that you need to update your schema: if you have named columns and non-utf8-or-bytes comparator if you have named columns but metadata shows more columns than names
          Hide
          Sylvain Lebresne added a comment -

          I'm not sure I understand what's a named columns above to be honest.

          There is basically two informations from CFMetadata you need to know to insert a column correctly in a table (CQL3 or no CQL3): the comparator and all of column_metadata. The comparator is necessary to know what is a valid column name and the column_metadata is necessary to know what is a valid column value (I'm simplifying a bit, I'm assuming that the key_validation and default_validator are BytesType but that doesn't matter for the problem at hand).

          Now the problem is that for any table created through CQL3 that doesn't use COMPACT STORAGE (let's call those CQL3 tables), all the ColumnDefinition of column_metada will have a componentIndex. So none of those ColumnDefinition are exposed in thrift. In practice it means that if I do:

          CREATE TABLE user {
              user_id blob PRIMARY KEY,
              name text,
              age int
          }
          

          then if a thrift client do a describe, it will basically get:

          comparator = CompositeType(UTF8Type) // it's a composite so that we can add collection later on
          column_metadata = []
          

          At that point we have two slightly separate problems:

          1. Even if a user produces a valid column, with say a composite name being "age" and a value being an int, then currently the code throw an exception. Fixing that exception is the goal of the attached patch (though it would have to be updated to work with collections in 1.2). I'm fine fixing that, though I'm pointing that there is a second, more general problem.
          2. Since the thrift client doesn't know about the actual column_metadata, how can we expect it to correctly insert data. In particular I'm pretty sure higher level clients like pycassa or astyanax will serialize data incorrectly if they don't know the right value validator. Besides, there is many way to be confused if you use a CQL3 table from thrift. For instance if you create the wrong column (i'ts enough to mess up the case), you'll be surprised to not be able to access it when you go back to CQL3. So be clear, I do am suggesting that we don't allow accessing table created from CQL3 without COMPACT STORAGE from thrift, because I think it will be more sane, even if it does mean that you're not coming back from CQL3 once you've start really using it.
          Show
          Sylvain Lebresne added a comment - I'm not sure I understand what's a named columns above to be honest. There is basically two informations from CFMetadata you need to know to insert a column correctly in a table (CQL3 or no CQL3): the comparator and all of column_metadata. The comparator is necessary to know what is a valid column name and the column_metadata is necessary to know what is a valid column value (I'm simplifying a bit, I'm assuming that the key_validation and default_validator are BytesType but that doesn't matter for the problem at hand). Now the problem is that for any table created through CQL3 that doesn't use COMPACT STORAGE (let's call those CQL3 tables), all the ColumnDefinition of column_metada will have a componentIndex. So none of those ColumnDefinition are exposed in thrift. In practice it means that if I do: CREATE TABLE user { user_id blob PRIMARY KEY, name text, age int } then if a thrift client do a describe, it will basically get: comparator = CompositeType(UTF8Type) // it's a composite so that we can add collection later on column_metadata = [] At that point we have two slightly separate problems: Even if a user produces a valid column, with say a composite name being "age" and a value being an int, then currently the code throw an exception. Fixing that exception is the goal of the attached patch (though it would have to be updated to work with collections in 1.2). I'm fine fixing that, though I'm pointing that there is a second, more general problem. Since the thrift client doesn't know about the actual column_metadata, how can we expect it to correctly insert data. In particular I'm pretty sure higher level clients like pycassa or astyanax will serialize data incorrectly if they don't know the right value validator. Besides, there is many way to be confused if you use a CQL3 table from thrift. For instance if you create the wrong column (i'ts enough to mess up the case), you'll be surprised to not be able to access it when you go back to CQL3. So be clear, I do am suggesting that we don't allow accessing table created from CQL3 without COMPACT STORAGE from thrift, because I think it will be more sane, even if it does mean that you're not coming back from CQL3 once you've start really using it.
          Hide
          Jonathan Ellis added a comment -

          Right, so what I was saying was, if we're willing to say that columndefinition name is always the cql3 column name, then we don't need componentIndex, at the price of potentially breaking a corner case in non-cql3 schemas.

          Show
          Jonathan Ellis added a comment - Right, so what I was saying was, if we're willing to say that columndefinition name is always the cql3 column name, then we don't need componentIndex, at the price of potentially breaking a corner case in non-cql3 schemas.
          Hide
          Sylvain Lebresne added a comment -

          But what the componentIndex give us is which of the composite component is the cql3 column name. Typically, with collections, it's not even necessarily the last of the component. Now, if you know the table is a CQL3 one, you could try to infer which component it is by saying that it's the last component, except if the last is a collection type, in which case that's the previous one, but that feels a bit messy. And besides, thrift client libraries don't have a simple automatic way to know if the table is a CQL3 one in the first place. I guess you could say that if you have a composite comparator and some column_metadata then you are likely a CQL3 table, but again, not very clean imo.

          At least internally I would be in favor of keeping the componentIndex as it is cleaner. I guess we could start returning the ColumnDefinition from CQL3 table without the componentIndex and let thrift client infer what they can. As said, I still think using CQL3 table from thrift has other way to be confusing, but why not.

          Show
          Sylvain Lebresne added a comment - But what the componentIndex give us is which of the composite component is the cql3 column name. Typically, with collections, it's not even necessarily the last of the component. Now, if you know the table is a CQL3 one, you could try to infer which component it is by saying that it's the last component, except if the last is a collection type, in which case that's the previous one, but that feels a bit messy. And besides, thrift client libraries don't have a simple automatic way to know if the table is a CQL3 one in the first place. I guess you could say that if you have a composite comparator and some column_metadata then you are likely a CQL3 table, but again, not very clean imo. At least internally I would be in favor of keeping the componentIndex as it is cleaner. I guess we could start returning the ColumnDefinition from CQL3 table without the componentIndex and let thrift client infer what they can. As said, I still think using CQL3 table from thrift has other way to be confusing, but why not.
          Hide
          Jonathan Ellis added a comment -

          Here's what I think our goals are, in order of importance:

          1. Existing "high level" clients should have a reasonable upgrade path to read and update collections and cql3 CFs.
          2. CLI and other tools that don't speak cql should have a way to tell that they can't cope with CQL3 CF definitions. This is the problem Nick described originally in this ticket description. Actually making such tools able to manipulate CQL3 definitions is NOT a goal, but we should, as Nick says, make that more obvious.
          3. We should allow updates to CQL3 CFs from Thrift, if someone manually composes the correct CompositeType bytes. This is what most of the rest of the discussion here involves.

          Analysis:

          1. This we have done--they will have to use cql-over-thrift, but IMO this is reasonable. Thrift RPC methods to deal with collections have never been on the roadmap.
          2. This is tough since if this involves new information (like exposing component_index, or even adding a cql3 boolean to CfDef) then old tools by definition won't know about it. Proposal: what if we omit CQL3 CfDefs from those we return to describe_keyspace[s] calls? Not quite as good as returning a CfDef that explicitly says "I am here but you can't touch this," but compare to displaying incomplete information (that the cli doesn't know is incomplete) it's much more obvious that the cli and other old thrift-base schema manipulators can't cope with such.
          3. Sylvain mentioned adding a thrift or JMX method to enable validation-free updates but I'd rather make ThriftValidation cql3-aware, which would let this work without any special flags. I don't see any downside to this except added complexity for ThriftValidation.
          Show
          Jonathan Ellis added a comment - Here's what I think our goals are, in order of importance: Existing "high level" clients should have a reasonable upgrade path to read and update collections and cql3 CFs. CLI and other tools that don't speak cql should have a way to tell that they can't cope with CQL3 CF definitions. This is the problem Nick described originally in this ticket description. Actually making such tools able to manipulate CQL3 definitions is NOT a goal, but we should, as Nick says, make that more obvious. We should allow updates to CQL3 CFs from Thrift, if someone manually composes the correct CompositeType bytes. This is what most of the rest of the discussion here involves. Analysis: This we have done--they will have to use cql-over-thrift, but IMO this is reasonable. Thrift RPC methods to deal with collections have never been on the roadmap. This is tough since if this involves new information (like exposing component_index, or even adding a cql3 boolean to CfDef) then old tools by definition won't know about it. Proposal: what if we omit CQL3 CfDefs from those we return to describe_keyspace [s] calls? Not quite as good as returning a CfDef that explicitly says "I am here but you can't touch this," but compare to displaying incomplete information (that the cli doesn't know is incomplete) it's much more obvious that the cli and other old thrift-base schema manipulators can't cope with such. Sylvain mentioned adding a thrift or JMX method to enable validation-free updates but I'd rather make ThriftValidation cql3-aware, which would let this work without any special flags. I don't see any downside to this except added complexity for ThriftValidation.
          Hide
          Jonathan Ellis added a comment -

          I do think we should push this to 1.2 however, since I'm leery of changing the behavior or describe_keyspace[s] when we are fairly deep into 1.1's stable lifetime.

          Show
          Jonathan Ellis added a comment - I do think we should push this to 1.2 however, since I'm leery of changing the behavior or describe_keyspace [s] when we are fairly deep into 1.1's stable lifetime.
          Hide
          Pavel Yaskevich added a comment -

          CQL3 defined CFs won't be exposed to Thrift API anymore (with warning), I also checked CassandraServer and ThriftValidation and figured that column name validation already in place via ThriftValidation.validateColumnNames(...).

          Show
          Pavel Yaskevich added a comment - CQL3 defined CFs won't be exposed to Thrift API anymore (with warning), I also checked CassandraServer and ThriftValidation and figured that column name validation already in place via ThriftValidation.validateColumnNames(...).
          Hide
          Nick Bailey added a comment -

          Just glanced at the patch, but it looks like this just makes cql3 cfs not show up in describe_keyspaces calls right? Reading/writing to the cf would still go through and error?

          If I'm looking at that right then it seems like we would want the opposite behavior. At least from my perspective, I would like to have an indication that the cf exists in my client even if i can't write to it.

          Show
          Nick Bailey added a comment - Just glanced at the patch, but it looks like this just makes cql3 cfs not show up in describe_keyspaces calls right? Reading/writing to the cf would still go through and error? If I'm looking at that right then it seems like we would want the opposite behavior. At least from my perspective, I would like to have an indication that the cf exists in my client even if i can't write to it.
          Hide
          Jonathan Ellis added a comment -

          The idea was to do two things:

          • update ThriftValidation to be cql3-aware, so if a Thrift user manually composes a valid column, we accept it
          • but, we don't expose cql3 CFs via describe_keyspaces since clients do not have enough information to generate valid requests automatically
          Show
          Jonathan Ellis added a comment - The idea was to do two things: update ThriftValidation to be cql3-aware, so if a Thrift user manually composes a valid column, we accept it but, we don't expose cql3 CFs via describe_keyspaces since clients do not have enough information to generate valid requests automatically
          Hide
          Sylvain Lebresne added a comment -

          I agree that the two points above (make ThriftValidation cql3-aware but don't expose cql3 CFs defs) make sense as a strategy.

          Show
          Sylvain Lebresne added a comment - I agree that the two points above (make ThriftValidation cql3-aware but don't expose cql3 CFs defs) make sense as a strategy.
          Hide
          Sylvain Lebresne added a comment -

          On Pavel's patch:

          • I'm not a fan of logging a warning (or to log anything really) if someone has CQL3 CFs. We're trying to push CQL3 as a good thing, let's not log anything that could be interpreted as if something was wrong/abnormal.
          • The check is excluding composite CF without any ColumnDefinition while it shouldn't.

          Attaching a v2 that:

          • Fix the two remarks above.
          • Rename the check as isThriftIncompatible. I think it's more about detecting CF definitions that cannot be exploided fully by thrift rather than discriminate between what is a thrift CF and CQL3 CF. Especially since the intersection between those two notions is not empty.
          • Ship the changes to ThriftValidation from my first patch, though modified a bit to be more generic and handle correctly collections. I'll note that this part makes validation potentially iterate over all ColumnDefinition for composite CF, but that's not really an issue since composite CF created on the thrift side are almost guaranteed to have no ColumnDefinition.
          Show
          Sylvain Lebresne added a comment - On Pavel's patch: I'm not a fan of logging a warning (or to log anything really) if someone has CQL3 CFs. We're trying to push CQL3 as a good thing, let's not log anything that could be interpreted as if something was wrong/abnormal. The check is excluding composite CF without any ColumnDefinition while it shouldn't. Attaching a v2 that: Fix the two remarks above. Rename the check as isThriftIncompatible. I think it's more about detecting CF definitions that cannot be exploided fully by thrift rather than discriminate between what is a thrift CF and CQL3 CF. Especially since the intersection between those two notions is not empty. Ship the changes to ThriftValidation from my first patch, though modified a bit to be more generic and handle correctly collections. I'll note that this part makes validation potentially iterate over all ColumnDefinition for composite CF, but that's not really an issue since composite CF created on the thrift side are almost guaranteed to have no ColumnDefinition.
          Hide
          Pavel Yaskevich added a comment -

          I'm not a fan of logging a warning (or to log anything really) if someone has CQL3 CFs. We're trying to push CQL3 as a good thing, let's not log anything that could be interpreted as if something was wrong/abnormal.

          Wouldn't that create confusion when some CFs are visible through Thrift and some are not or do we rely on that users should know what is so special about CQL3 CFs?

          Show
          Pavel Yaskevich added a comment - I'm not a fan of logging a warning (or to log anything really) if someone has CQL3 CFs. We're trying to push CQL3 as a good thing, let's not log anything that could be interpreted as if something was wrong/abnormal. Wouldn't that create confusion when some CFs are visible through Thrift and some are not or do we rely on that users should know what is so special about CQL3 CFs?
          Hide
          Sylvain Lebresne added a comment -

          Wouldn't that create confusion when some CFs are visible through Thrift and some are not

          I'm not saying this shouldn't be documented at all, but merely that it's a documentation issue and as such logging it at each startup is not the right place.

          Show
          Sylvain Lebresne added a comment - Wouldn't that create confusion when some CFs are visible through Thrift and some are not I'm not saying this shouldn't be documented at all, but merely that it's a documentation issue and as such logging it at each startup is not the right place.
          Hide
          Pavel Yaskevich added a comment -

          If everyone else is ok with that, lgtm.

          Show
          Pavel Yaskevich added a comment - If everyone else is ok with that, lgtm.
          Hide
          Jonathan Ellis added a comment -

          So, this is good to commit?

          Show
          Jonathan Ellis added a comment - So, this is good to commit?
          Hide
          Sylvain Lebresne added a comment -

          Alright, committed, thanks.

          Show
          Sylvain Lebresne added a comment - Alright, committed, thanks.

            People

            • Assignee:
              Pavel Yaskevich
              Reporter:
              Nick Bailey
              Reviewer:
              Sylvain Lebresne
            • Votes:
              2 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development