[jira] [Commented] (CASSANDRA-13994) Remove COMPACT STORAGE internals before 4.0 release

["Sylvain Lebresne (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CASSANDRA-13994?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17129271#comment-17129271 ] 

Sylvain Lebresne commented on CASSANDRA-13994:
----------------------------------------------

Looked at the patch. I left a few nitpicks [on this commit|https://github.com/pcmanus/cassandra/commit/0ab608cbb6f78657ae6b3e99cea0f47d84aa3dad]. More general remarks/questions:
 * I'd be in favor of removing support for the native protocol V3 while at it. V4 has been out since pre-C* 3.0, and V3 still use the cell layout in the paging state, which force us to keep some legacy code around like {{CellInLegacyOrderIterator}} in {{BTreeRow.java}} (plus, some complexity in {{PagingState}} can be nixed if we remove it). Overall, I find it doubtful that anyone would still be on V3 on 3.X+, but even if someone is, I think forcing them to upgrade to V4 pre-upgrade to 4.0 is a good idea even outside of the benefit of allowing us to remove some code.
 * In {{TableMetadata:}}
 ** I think we should remove the {{#isCompound}} method, as only compact tables could ever be non-compound, so "compound" does not make sense anymore. Also, we shouldn't remove the {{&& flags.contains(Flag.COMPOUND)}} part from {{Flag#isSupported}}, as a non-compound table is _not_ supported anymore and is indicative of someone forgetting to use {{DROP COMPACT STORAGE}}.
 ** We should however keep the {{CompactTable#isSuperColumnMapColumn}} method and its call (though it's probably not worth keeping the {{CompactTables}} class for that; a static method in {{TableMetadata}} is probably fine; I pushed a commit doing just that [here|https://github.com/pcmanus/cassandra/commit/0ab608cbb6f78657ae6b3e99cea0f47d84aa3dad] if you'd like) .
 * In {{SchemaEvent.java}}, in {{repr(TableMetadata)}}, we should keep the "isCounter" case (we still support counter tables). However, we should remove the "isCompound" entry (for the reason mentioned above).
 * In {{ColumnFamilyStore}}, at the end of the ctor, the code to detect unsupported indexes has been commented out. Is that intentional?
 * Maybe worth removing the references to {{COMPACT STORAGE}} in the doc, namely those in {{doc/source/cql/ddl.rst}}.
 * I'm a bit unsure what our story about upgrading KEYS indexes is, and in particular, I'm unsure we can remove them like that. That is, while we could ask users to drop their KEYS index and re-create them afterwards, this cannot be done without downtime (for anything touching the index), and are we OK with that?

{quote}About the system table Built_indexes, I saw there is some SystemKeyspaceMigrator40 class, I guess on start we can check and migrate this table there (if it was not already migrated) ?
{quote}
That's an option, and I'm ok if that's done. But fwiw, I'm equally ok with letting it be. Basically, that table simply has a {{value}} column that is unused, but as that table is meant for internal consumption in the first place, that feel like a pretty minor detail. So either way is fine with me.
{quote}We definitely need to be very careful when removing usage of compact storage methods, since most of the usage is in around the storage engine.
{quote}
I wanted to point out that all the hard, deep, storage engine level changes have been done when removing thrift, some times ago. The patch here is actually pretty simple and almost exclusively touch CQL. It's also pretty easy to convince oneself that the majority of the change is just removing dead code.

The one exception imo is the 2ndary index question, and that's worth being careful, but the rest is pretty straightforward.
{quote}Also, it might be useful to know the impact of the removal and whether or not we got anything from it performance-wise
{quote}
Related to my previous point, I'm cool if we do performance testing, that never hurt, but I think this is *very* low on the list of tickets that justify the effort. Again, we're merely removing a bunch of 'if' in CQL that are never taken anymore, so the performance impact is almost surely non measurable, one way or another.

> Remove COMPACT STORAGE internals before 4.0 release
> ---------------------------------------------------
>
>                 Key: CASSANDRA-13994
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-13994
>             Project: Cassandra
>          Issue Type: Improvement
>          Components: Legacy/Local Write-Read Paths
>            Reporter: Alex Petrov
>            Assignee: Ekaterina Dimitrova
>            Priority: Low
>             Fix For: 4.0, 4.0-rc
>
>
> 4.0 comes without thrift (after [CASSANDRA-11115]) and COMPACT STORAGE (after [CASSANDRA-10857]), and since Compact Storage flags are now disabled, all of the related functionality is useless.
> There are still some things to consider:
> 1. One of the system tables (built indexes) was compact. For now, we just added {{value}} column to it to make sure it's backwards-compatible, but we might want to make sure it's just a "normal" table and doesn't have redundant columns.
> 2. Compact Tables were building indexes in {{KEYS}} mode. Removing it is trivial, but this would mean that all built indexes will be defunct. We could log a warning for now and ask users to migrate off those for now and completely remove it from future releases. It's just a couple of classes though.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

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