Mailing-List: contact commits-help@cassandra.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cassandra.apache.org
Date: Tue, 26 Apr 2016 13:29:12 +0000 (UTC)
From: "Sylvain Lebresne (JIRA)" <jira@apache.org>
To: commits@cassandra.apache.org
Message-ID: <JIRA.12956207.1459874727000.34516.1461677352918@Atlassian.JIRA>
In-Reply-To: <JIRA.12956207.1459874727000@Atlassian.JIRA>
References: <JIRA.12956207.1459874727000@Atlassian.JIRA>
 <JIRA.12956207.1459874727165@arcas>
Subject: [jira] [Commented] (CASSANDRA-11502) Fix denseness and column
 metadata updates coming from Thrift
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


    [ https://issues.apache.org/jira/browse/CASSANDRA-11502?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=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&page=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)