[jira] [Commented] (CASSANDRA-11502) Fix denseness and column metadata updates coming from Thrift
[ https://issues.apache.org/jira/browse/CASSANDRA-11502?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15260482#comment-15260482 ] Aleksey Yeschenko commented on CASSANDRA-11502: --- Committed as [e5c40278001bf3a9582085a58941e5f4765f118c|https://github.com/apache/cassandra/commit/e5c40278001bf3a9582085a58941e5f4765f118c] to 2.2 and merged with 3.0 and trunk, thanks. Did some manual testing w/ cqlsh/nodetool to make sure sparse CFs w/ clustering columns don't pass {{isThriftCompatibleTest()}}, and it seems like we are all good. > Fix denseness and column metadata updates coming from Thrift > > > Key: CASSANDRA-11502 > URL: https://issues.apache.org/jira/browse/CASSANDRA-11502 > Project: Cassandra > Issue Type: Bug > Components: Distributed Metadata >Reporter: Aleksey Yeschenko >Assignee: Aleksey Yeschenko >Priority: Minor > Fix For: 2.2.x, 3.0.x, 3.x > > > It was > [decided|https://issues.apache.org/jira/browse/CASSANDRA-7744?focusedCommentId=14095472=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14095472] > that we'd be recalculating {{is_dense}} for table updates coming from Thrift > on every change. However, due to some oversight, {{is_dense}} can only go > from {{false}} to {{true}}. Once dense, even adding a {{REGULAR}} column will > not reset {{is_dense}} back to {{false}}. > The recalculation fails because no matter what happens, we never remove the > auto-generated {{CLUSTERING}} and {{COMPACT_VALUE}} columns of a dense table. > Which ultimately leads to the issue on 2.2 to 3.0 upgrade (see > CASSANDRA-11315). > What we should do is remove the special-case for Thrift in > {{LegacySchemaTables::makeUpdateTableMutation}} and correct the logic in > {{ThriftConversion::internalFromThrift}} to remove those columns when going > from dense to sparse. > This is not enough to fix CASSANDRA-11315, however, as we need to handle > pre-patch upgrades, and upgrades from 2.1. Fixing it in 2.2 means a) getting > proper schema from {{DESCRIBE}} now and b) using the more efficient > {{SparseCellNameType}} when you add columns. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-11502) Fix denseness and column metadata updates coming from Thrift
[ https://issues.apache.org/jira/browse/CASSANDRA-11502?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15260146#comment-15260146 ] Aleksey Yeschenko commented on CASSANDRA-11502: --- bq. I help someone a few days ago with an upgrade problem and was able to do an update on a CQL table, but that was on some 2.0 version so must have been on some version from before we introduced that. Do you have that table schema handy? I might as well check if the check for that fails in 2.1+ and open a new ticket if so. > Fix denseness and column metadata updates coming from Thrift > > > Key: CASSANDRA-11502 > URL: https://issues.apache.org/jira/browse/CASSANDRA-11502 > Project: Cassandra > Issue Type: Bug > Components: Distributed Metadata >Reporter: Aleksey Yeschenko >Assignee: Aleksey Yeschenko >Priority: Minor > Fix For: 2.2.x, 3.0.x, 3.x > > > It was > [decided|https://issues.apache.org/jira/browse/CASSANDRA-7744?focusedCommentId=14095472=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14095472] > that we'd be recalculating {{is_dense}} for table updates coming from Thrift > on every change. However, due to some oversight, {{is_dense}} can only go > from {{false}} to {{true}}. Once dense, even adding a {{REGULAR}} column will > not reset {{is_dense}} back to {{false}}. > The recalculation fails because no matter what happens, we never remove the > auto-generated {{CLUSTERING}} and {{COMPACT_VALUE}} columns of a dense table. > Which ultimately leads to the issue on 2.2 to 3.0 upgrade (see > CASSANDRA-11315). > What we should do is remove the special-case for Thrift in > {{LegacySchemaTables::makeUpdateTableMutation}} and correct the logic in > {{ThriftConversion::internalFromThrift}} to remove those columns when going > from dense to sparse. > This is not enough to fix CASSANDRA-11315, however, as we need to handle > pre-patch upgrades, and upgrades from 2.1. Fixing it in 2.2 means a) getting > proper schema from {{DESCRIBE}} now and b) using the more efficient > {{SparseCellNameType}} when you add columns. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-11502) Fix denseness and column metadata updates coming from Thrift
[ https://issues.apache.org/jira/browse/CASSANDRA-11502?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15260110#comment-15260110 ] Sylvain Lebresne commented on CASSANDRA-11502: -- bq. but I, instead, feel more paranoid about leaving it in Fair enough, I'm good getting rid of it. bq. I think we should be safe here b/c of the {{isThriftCompatible()}} guard in {{CassandraServer::system_update_column_family()}}. You're right. I got confused because I help someone a few days ago with an upgrade problem and was able to do an update on a CQL table, but that was on some 2.0 version so must have been on some version from before we introduced that. Would still be great to double check but +1 on the patch in any case. > Fix denseness and column metadata updates coming from Thrift > > > Key: CASSANDRA-11502 > URL: https://issues.apache.org/jira/browse/CASSANDRA-11502 > Project: Cassandra > Issue Type: Bug > Components: Distributed Metadata >Reporter: Aleksey Yeschenko >Assignee: Aleksey Yeschenko >Priority: Minor > Fix For: 2.2.x, 3.0.x, 3.x > > > It was > [decided|https://issues.apache.org/jira/browse/CASSANDRA-7744?focusedCommentId=14095472=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14095472] > that we'd be recalculating {{is_dense}} for table updates coming from Thrift > on every change. However, due to some oversight, {{is_dense}} can only go > from {{false}} to {{true}}. Once dense, even adding a {{REGULAR}} column will > not reset {{is_dense}} back to {{false}}. > The recalculation fails because no matter what happens, we never remove the > auto-generated {{CLUSTERING}} and {{COMPACT_VALUE}} columns of a dense table. > Which ultimately leads to the issue on 2.2 to 3.0 upgrade (see > CASSANDRA-11315). > What we should do is remove the special-case for Thrift in > {{LegacySchemaTables::makeUpdateTableMutation}} and correct the logic in > {{ThriftConversion::internalFromThrift}} to remove those columns when going > from dense to sparse. > This is not enough to fix CASSANDRA-11315, however, as we need to handle > pre-patch upgrades, and upgrades from 2.1. Fixing it in 2.2 means a) getting > proper schema from {{DESCRIBE}} now and b) using the more efficient > {{SparseCellNameType}} when you add columns. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-11502) Fix denseness and column metadata updates coming from Thrift
[ https://issues.apache.org/jira/browse/CASSANDRA-11502?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15258318#comment-15258318 ] Aleksey Yeschenko commented on CASSANDRA-11502: --- Re 1: If you leave it alone, then {{is_dense}} will forever be stuck at true and never recomputed. This is breaking upgrading to 3.0 (now, we'll still need to address this in 3.0, for users upgrading from C* that don't have this patch in, but that's what CASSANDRA-11315 is for). Now, I 100% get the caution, but I, instead, feel more paranoid about leaving it in - having the logic duplicated in two places has already led to this one bug. And, a lot less critically, in preparation for Thrift removal I want to push any Thrift code as close to the edges as possible. Re 2: I *think* we should be safe here b/c of the {{isThriftCompatible()}} guard in {{CassandraServer::system_update_column_family()}}. If we aren't, then we have a bug in {{update_column_family()}}. I'll double check. > Fix denseness and column metadata updates coming from Thrift > > > Key: CASSANDRA-11502 > URL: https://issues.apache.org/jira/browse/CASSANDRA-11502 > Project: Cassandra > Issue Type: Bug > Components: Distributed Metadata >Reporter: Aleksey Yeschenko >Assignee: Aleksey Yeschenko >Priority: Minor > Fix For: 2.2.x, 3.0.x, 3.x > > > It was > [decided|https://issues.apache.org/jira/browse/CASSANDRA-7744?focusedCommentId=14095472=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14095472] > that we'd be recalculating {{is_dense}} for table updates coming from Thrift > on every change. However, due to some oversight, {{is_dense}} can only go > from {{false}} to {{true}}. Once dense, even adding a {{REGULAR}} column will > not reset {{is_dense}} back to {{false}}. > The recalculation fails because no matter what happens, we never remove the > auto-generated {{CLUSTERING}} and {{COMPACT_VALUE}} columns of a dense table. > Which ultimately leads to the issue on 2.2 to 3.0 upgrade (see > CASSANDRA-11315). > What we should do is remove the special-case for Thrift in > {{LegacySchemaTables::makeUpdateTableMutation}} and correct the logic in > {{ThriftConversion::internalFromThrift}} to remove those columns when going > from dense to sparse. > This is not enough to fix CASSANDRA-11315, however, as we need to handle > pre-patch upgrades, and upgrades from 2.1. Fixing it in 2.2 means a) getting > proper schema from {{DESCRIBE}} now and b) using the more efficient > {{SparseCellNameType}} when you add columns. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-11502) Fix denseness and column metadata updates coming from Thrift
[ https://issues.apache.org/jira/browse/CASSANDRA-11502?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15258085#comment-15258085 ] Sylvain Lebresne commented on CASSANDRA-11502: -- Mostly looks good with the following remarks: * Regarding the cleanup of {{SchemaKeyspace::makeUpdateTableMutation}} (in both 2.2 and 3.0), I follow the reasoning and I'm relatively sure you're right, but was that breaking anything? I mention this because those schema upgrade and thrift stuffs are easily subtle and are unfortunately not very well covered by tests, so while this does look an ok removal, I'd hate to introduce some subtle upgrade bug just to remove a few lines of code that will go away in 4.0. Anyway, I'm not opposing the removal per-se, but just mentioning that if it was completely up to me, I'd probably leave them in out of (probably unreasonably extreme) caution. * I believe the removal of {{CLUSTERING_COLUMN}} when sparse table could screw up some CQL tables. Now, I agree that if someone modifies a CQL table from thrift he is obviously looking for trouble, but it shouldn't be too hard to avoid the problem by only excluding the column based on its {{position()}} (only if it correspond to the last componenent of the comparator, if that comparator is composite). Besides, the patch modify the code to preserve {{STATIC}} column, which should only ever matters for CQL table so it sounds like you do care about that case :) > Fix denseness and column metadata updates coming from Thrift > > > Key: CASSANDRA-11502 > URL: https://issues.apache.org/jira/browse/CASSANDRA-11502 > Project: Cassandra > Issue Type: Bug > Components: Distributed Metadata >Reporter: Aleksey Yeschenko >Assignee: Aleksey Yeschenko >Priority: Minor > Fix For: 2.2.x, 3.0.x, 3.x > > > It was > [decided|https://issues.apache.org/jira/browse/CASSANDRA-7744?focusedCommentId=14095472=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14095472] > that we'd be recalculating {{is_dense}} for table updates coming from Thrift > on every change. However, due to some oversight, {{is_dense}} can only go > from {{false}} to {{true}}. Once dense, even adding a {{REGULAR}} column will > not reset {{is_dense}} back to {{false}}. > The recalculation fails because no matter what happens, we never remove the > auto-generated {{CLUSTERING}} and {{COMPACT_VALUE}} columns of a dense table. > Which ultimately leads to the issue on 2.2 to 3.0 upgrade (see > CASSANDRA-11315). > What we should do is remove the special-case for Thrift in > {{LegacySchemaTables::makeUpdateTableMutation}} and correct the logic in > {{ThriftConversion::internalFromThrift}} to remove those columns when going > from dense to sparse. > This is not enough to fix CASSANDRA-11315, however, as we need to handle > pre-patch upgrades, and upgrades from 2.1. Fixing it in 2.2 means a) getting > proper schema from {{DESCRIBE}} now and b) using the more efficient > {{SparseCellNameType}} when you add columns. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-11502) Fix denseness and column metadata updates coming from Thrift
[ https://issues.apache.org/jira/browse/CASSANDRA-11502?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15230525#comment-15230525 ] Aleksey Yeschenko commented on CASSANDRA-11502: --- ||branch||testall||dtest|| |[11502-2.2|https://github.com/iamaleksey/cassandra/tree/11502-2.2]|[testall|http://cassci.datastax.com/view/Dev/view/iamaleksey/job/iamaleksey-11502-2.2-testall]|[dtest|http://cassci.datastax.com/view/Dev/view/iamaleksey/job/iamaleksey-11502-2.2-dtest]| |[11502-3.0|https://github.com/iamaleksey/cassandra/tree/11502-3.0]|[testall|http://cassci.datastax.com/view/Dev/view/iamaleksey/job/iamaleksey-11502-3.0-testall]|[dtest|http://cassci.datastax.com/view/Dev/view/iamaleksey/job/iamaleksey-11502-3.0-dtest]| 2.2 version makes sure we recalculate and remove redundant column. 3.0 versions preserves denseness, but cleans up unnecessary Thrift special casing from {{SchemaKeyspace::makeUpdateTableMutation}}. It's unnecessary because despite Thrift not knowing about non regular/static columns, {{ThriftConversion::internalFromThrift}} will include them from the current CFM, so we are not going to lose anything. > Fix denseness and column metadata updates coming from Thrift > > > Key: CASSANDRA-11502 > URL: https://issues.apache.org/jira/browse/CASSANDRA-11502 > Project: Cassandra > Issue Type: Bug > Components: Distributed Metadata >Reporter: Aleksey Yeschenko >Assignee: Aleksey Yeschenko >Priority: Minor > Fix For: 2.2.x, 3.0.x, 3.x > > > It was > [decided|https://issues.apache.org/jira/browse/CASSANDRA-7744?focusedCommentId=14095472=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14095472] > that we'd be recalculating {{is_dense}} for table updates coming from Thrift > on every change. However, due to some oversight, {{is_dense}} can only go > from {{false}} to {{true}}. Once dense, even adding a {{REGULAR}} column will > not reset {{is_dense}} back to {{false}}. > The recalculation fails because no matter what happens, we never remove the > auto-generated {{CLUSTERING}} and {{COMPACT_VALUE}} columns of a dense table. > Which ultimately leads to the issue on 2.2 to 3.0 upgrade (see > CASSANDRA-11315). > What we should do is remove the special-case for Thrift in > {{LegacySchemaTables::makeUpdateTableMutation}} and correct the logic in > {{ThriftConversion::internalFromThrift}} to remove those columns when going > from dense to sparse. > This is not enough to fix CASSANDRA-11315, however, as we need to handle > pre-patch upgrades, and upgrades from 2.1. Fixing it in 2.2 means a) getting > proper schema from {{DESCRIBE}} now and b) using the more efficient > {{SparseCellNameType}} when you add columns. -- This message was sent by Atlassian JIRA (v6.3.4#6332)