[
https://issues.apache.org/jira/browse/CASSANDRA-9425?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15826564#comment-15826564
]
Aleksey Yeschenko commented on CASSANDRA-9425:
----------------------------------------------
Alright. Fixed the failing tests ({{TableId}} patch broke Paxos, and index
metadata patch altered custom index handling a bit - for the better - but broke
{{CustomIndexTests}}), and addressed some of the comments.
In addition got rid of {{DataResource}} in {{TableMetadata}}, and undid a tiny
bit of of the related CASSANDRA-10410 optimisation I hope you don’t mind.
bq. I'd rename {{TableMetadata.table}} to {{TableMetadata.name}} for
consistency with other classes ({{KeyspaceMetadata}} and {{IndexMetadata}} at
least). While at it, I'd also rename {{ksName}} to {{keyspace}} and
{{viewName}} to {{name}} in {{ViewMetadata}}.
Both done.
bq. I'd also probably rename {{PartitionColumns}} to
{{RegularAndStaticColumns}} now.
Done.
bq. Nit: In {{SSTableTxnWriter}}, it feels slightly inconsistent that
{{createRangeAware}} takes a {{TableMetadata}}, but create takes a
{{TableMetadataRef}}.
I try to use {{TableMetadata}} directly whenever possible, only pass the Ref
when unavoidable. In this case it’s no big deal, though, so done.
bq. In {{Indexes.validate()}}, not sure it's worth doing the duplicate name
check since we do it at the keyspace level already (and only doing it within a
single table is a false sense of safety).
Problem is, in announce for {{CREATE INDEX}} we do not call {{KSM.validate()}},
yet. So for the time being, until the next patch, where we always validate the
entirety of the resulting schema
before applying anything, it has to stay here, even if imperfect. Removing it
will fail some tests. And some checks - for now - are still better than no
checks. Verdict: left be until the next patch.
bq. I'd rename {{addRegularColumn}} and {{alterColumnType}} in {{ViewMetadata}}
as they are not modifying the {{ViewMetadata}} but creating copies (say to
{{withAddedRegularColumn()}} and {{withAlteredColumnType}}).
Done.
bq. Is there a rational for notifying everything at the end, versus calling
each notification in the "appropriate" sub-method (notifying table alter in
{{alterTable()}}, ....)? The later would feels a tad more readable to me as
it's slightly easier to check we haven't forgotten something.
I prefer it stylistically. But I also want to get schema and related DB objects
({{Keyspace}}, {{CFS}}, and everything downstream) to get to the consistent
state as fast as possible. For that reason
I’m delaying potentially blocking change event handlers until the very end. On
that note, we should probably eventually move their processing elsewhere, away
from the sensitive path - just enqueue them into some queue and have something
poll and process the queue from time to time.
bq. I'd maybe suggest adding a private {{handleDiff(mapDiff, Function
onDropped, Function onAdded, Function onUpdated)}} method to simplify a bit.
It’s a very minor case of duplication that doesn’t worry me, I’d rather not
factor it wouldn’t strictly speaking make things simpler. Also see the previous
point.
bq. There is a TODO FIXME in {{SchemaKeyspace}} (and not 100% sure what it's
about).
It’s an ugly pre-existing piece of code to avoid re-compiling the UDFs; It
annoys me immensely that a method in {{SchemaKeyspace}} is referencing
{{Schema.instance}}, hence the TODO. I want to get rid of it eventually, but
it’s not important enough to spend my time on it atm.
bq. There is TODO: FIXME in {{ColumnFamilyStore.setCompressionParameters}}.
See CASSANDRA-12949. The method in its current implementation (1.1+) is broken
and unsafe, so I’ve disabled it for safety reasons. Hopefully someone will get
around to dealing with it some time, but I don’t have the time. Either way,
committed a clarification for that TODO.
bq. {{Keyspaces has both a {{get(String)}} that return {{Optional}} and a
{{getNullable(String)}}. We should probably remove one.
Not just {{Keyspaces}}. I prefer to have both options available, as there are
multiple things that can tolerate null return. The alternative would be using
{{get(name).orElse(null)}}, which I personally find unappealing.
bq. The patch changes some system table options (compared to current trunk)
I removed the explicit setting or {{readRepairChance}} to 0.0 as it was
redundant. We made {{readRepairChance}} default to be 0.0 a while ago. Only
{{dcLocalReadRepairChance}} needs resetting.
{{system_distributed}} was switched to default gc gs because gc gs of 0 is a
bug. {{view_build_status}} table accepts deletes, which can be compacted away
before propagation with 0 gc gs. See CASSANDRA-12954. Other tables in that
keyspace should be unaffected, as they don’t use TTL nor explicit deletes.
As for {{system_auth}}, it was never intended to have a special memtable flush
period setting, it was just an accidental reuse.
Should’ve probably commented on all of these 3 in patch comments, but all of
these changes are intentional. Sorry.
bq. I'm unclear on the changes to {{AlterTypeStatement}}. The patch removes the
code that propagate type changes to other table/view/types, but I neither see
where that has moved, nor why that wouldn't be needed anymore (as an aside, if
it does is not needed anymore, than we could remove the {{updateTypes()}} and
{{updateWith()}} methods that are unused in the patch).
They aren’t needed because since 3.0 we started using only the CQL type name in
all system tables. As such, we do not need to update individual UDTs, UDFs,
UDAs, tables, and views. Propagating the type change alone is sufficient. When
the merge happens, the difference will be picked up anyway, and the affected
tables/types/etc. will also be reloaded. Should’ve removed this in 3.0 schema
rewrite.
Removed {{updateTypes()}} and {{updateWith()}} though, thanks for noticing
(IDEA didn’t as they call each other recursively).
Will address the remaining few points shortly, but this is ready for a minor
follow up review. Thank you.
> Make node-local schema fully immutable
> --------------------------------------
>
> Key: CASSANDRA-9425
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9425
> Project: Cassandra
> Issue Type: Sub-task
> Reporter: Aleksey Yeschenko
> Assignee: Aleksey Yeschenko
> Fix For: 4.0
>
>
> The way we handle schema changes currently is inherently racy.
> All of our {{SchemaAlteringStatement}} s perform validation on a schema state
> that's won't necessarily be there when the statement gets executed and
> mutates schema.
> We should make all the *Metadata classes ({{KeyspaceMetadata,
> TableMetadata}}, {{ColumnMetadata}}, immutable, and local schema persistently
> snapshottable, with a single top-level {{AtomicReference}} to the current
> snapshot. Have DDL statements perform validation and transformation on the
> same state.
> In pseudo-code, think
> {code}
> public interface DDLStatement
> {
> /**
> * Validates that the DDL statement can be applied to the provided schema
> snapshot.
> *
> * @param schema snapshot of schema before executing CREATE KEYSPACE
> */
> void validate(SchemaSnapshot schema);
>
> /**
> * Applies the DDL statement to the provided schema snapshot.
> * Implies that validate() has already been called on the provided
> snapshot.
> *
> * @param schema snapshot of schema before executing the statement
> * @return snapshot of schema as it would be after executing the statement
> */
> SchemaSnapshot transform(SchemaSnapshot schema);
> }
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
