[
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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
