cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Aleksey Yeschenko (JIRA)" <>
Subject [jira] [Commented] (CASSANDRA-13426) Make all DDL statements idempotent and not dependent on global state
Date Fri, 20 Apr 2018 14:05:00 GMT


Aleksey Yeschenko commented on CASSANDRA-13426:

Alright, this should be more or less it. Addressed everything, only commenting on points not
addressed in code:

bq. TableMetadata/TableParams/Views / Maybe a bit subjective, but I find the naming of "unbuild"
unintuitive. How about something like builderFrom/makeBuilder/asBuilder/toBuilder

I actually like it, as it’s a direct antonym of ‘build’ and does the exact opposite
of what build() does. Couldn’t find a better antonym verb, and not a fan of any of the suggestions
above, so I’ll be leaveing it as is.

bq. TableMetadata / toDebugString is unused

It’s just a helper method for debugging. We have a few similar ones, none of them used by
code. It’s by design.

bq. Keyspaces / get(String name) is unused

It isn’t, but no harm in leaving it be. Similar container classes all have it.

bq. KeyspaceDiff / A comment explaining why function diffs need to be handled differently
could be useful (I'm assuming it's because the filtering to distinguish UDFs/UDAs makes it
slightly more expensive than the other types of diff).

Not sure what you mean. Why they are separate fields? Because from schema perspective they
are very different categories, and it’s just more helpful for consumers of of KeyspaceDiff
to have them as separate fields than as one.

bq. Functions / can Filter.test use isAggregate rather than instanceof?

It could, but test for UDF would still have to use instanceof. So I’d rather keep instanceof
there for UDA for consistency.

bq. CompressionParams / outstanding TODO on setCrcCheckChance

Removing it is beyond this JIRA (although it didn’t stop quite a few other things from being
included). For our purposes it’s harmless-ish, as it doesn’t count for hashCode() and
equals() purposes and doesn’t mess with schema. Ultimately that field should be removed,
so I left another comment. 

bq. SetType/ListType/MapType/DynamicCompositeType / getInstance can be simplified to just
return the result of computeIfAbsent. There's an unchecked warning, but I'm not sure that's
any different from the existing one. ReversedType and CompositeType impls already do this.

The extra check is there to avoid contention on the hot path. Updated ReversedType and CompositeType
to also try a get() first.

bq. UDAggregate / When reconstructing from schema tables and the function can't be reconstructed
for whatever reason - we preserve the old behaviour with a dummy, broken function for UDFs
but not for UDAs. These now trigger an assert. Is this an issue?

A UDA has no body of its own, it’s just a combination of UDFs. As such, when we try to assemble
a UDA from rows on disk, and one or more UDFs are missing, we treat this as an error and abort.
An analogy: deserializing table metadata from schema tables with a column that references
a non-existent UDT. A UDF OTOH can be correct, from schema perspective, but fail to compile
for a variety of reasons (think internals having changed on upgrade). In that case we still
want to start up, so we manufacture a broken function stub. But, again, for UDA we shouldn’t.
That said, I updated the code to throw a more verbose and helpful exception.

bq. UFTest / line #114 seems to have a typo - s/string1/string

Not a typo. Body needs to be different or else the UDF won’t be updated.

> Make all DDL statements idempotent and not dependent on global state
> --------------------------------------------------------------------
>                 Key: CASSANDRA-13426
>                 URL:
>             Project: Cassandra
>          Issue Type: Sub-task
>          Components: Distributed Metadata
>            Reporter: Aleksey Yeschenko
>            Assignee: Aleksey Yeschenko
>            Priority: Major
>             Fix For: 4.0
> A follow-up to CASSANDRA-9425 and a pre-requisite for CASSANDRA-10699.
> It's necessary for the latter to be able to apply any DDL statement several times without
side-effects. As part of the ticket I think we should also clean up validation logic and our
error texts. One example is varying treatment of missing keyspace for DROP TABLE/INDEX/etc.
statements with IF EXISTS.

This message was sent by Atlassian JIRA

To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message