[
https://issues.apache.org/jira/browse/CASSANDRA-13426?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16445765#comment-16445765
]
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: https://issues.apache.org/jira/browse/CASSANDRA-13426
> 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
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
