cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sylvain Lebresne (JIRA)" <>
Subject [jira] [Commented] (CASSANDRA-9425) Make node-local schema fully immutable
Date Wed, 18 Jan 2017 09:50:26 GMT


Sylvain Lebresne commented on CASSANDRA-9425:

bq. and undid a tiny bit of of the related CASSANDRA-10410 optimisation I hope you don’t

Well, actually, your motivations on this are lost on me. I'm certainly not pretending that
optimization was the most important thing ever, but as mentioned in the original ticket it
did show up in some profiling and the "optimization" was beyond tiny and entirely encapsulated
within the class. As an aside, also not a fan of manually writting hashcode() methods when
the simpler Objects.hashcode() can be and was used.

bq. 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.

Wfm, though it sounds like something we could add as comment in the code, so we remember what's
up in the unlikely even that "next patch" forgets about this (and so I don't have to ask about
this removal when reviewing the next patch since I'll most likely will have forgot about it).

bq. we should probably eventually move their processing elsewhere, away from the sensitive

I don't necessarily disagree but I don't think it's a big problem right now so I'd suggest
keeping it simple for now.

bq. 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.

I think that, all other things being equal, terser and easier to read code *does* make things
simpler, even if it's a tiny bit so, so I still stand by my suggestion. But it's certainly
small and probably up to personal preferences, so ok.

bq. 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

Fair enough, but could you update the TODO with this expanded and useful context.

bq. The alternative would be using {{get(name).orElse(null)}}

Well, that's one alternative. The other, which I'd personally would go with, is to remove
the method that return {{Optional}} altogether. {{Optional}} is useless outside of adding
clarity to the fact there may be no result, but naming the method {{getNullable}} is good
enough on that front imo.

Thanks for the explanations on the rest.

> Make node-local schema fully immutable
> --------------------------------------
>                 Key: CASSANDRA-9425
>                 URL:
>             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

View raw message