CASSANALYTICS-182 : CDC batch-write mixing a CDC-enabled and CDC-disabled table drops the CDC table's mutation#224
Conversation
| @MethodSource("org.apache.cassandra.bridge.VersionRunner#bridges") | ||
| public void testCqlTableCdcFlagSurvivesSerialization(CassandraBridge bridge) | ||
| { | ||
| // CqlTable.equals()/hashCode() deliberately exclude the cdc field (see CqlTable.java), |
There was a problem hiding this comment.
Why don't we have the cdc flag as part of our equality comparison and hashcode impl?
| // table has cdc enabled already, update schema if it has changed | ||
| LOGGER.debug("CDC already enabled keyspace={} table={}", table.keyspace(), table.table()); | ||
| // table already registered in Schema.instance — update if schema changed | ||
| LOGGER.debug("Table already in Schema.instance, updating if changed keyspace={} table={}", table.keyspace(), table.table()); |
There was a problem hiding this comment.
Since this can noop presumably on a lot of cases, I think we should consider logging in the maybeUpdateSchema call in the case where we have an update. The prior logging here made sense as it was a dupe process; here it's just telling us that we already knew a table existed but not much else.
| Preconditions.checkArgument(CassandraSchema.isCdcEnabled(schema, table), | ||
| "CDC not enabled for table: " + table.keyspace() + "." + table.table()); | ||
| CassandraSchema.maybeUpdateSchema(schema, partitioner, table, tableId, table.cdc()); | ||
| continue; |
There was a problem hiding this comment.
Why the if / continue structure (which is arguably glorified modern-day goto) instead of an if / else if / else structure?
There was a problem hiding this comment.
This confused me. The name cdcEnabledTables followed by:
cdcEnabledTables.forEach((ks, tables) -> tables.forEach(table -> {
LOGGER.warn("Disabling CDC on table keyspace={} table={}", ks, table);
CassandraSchema.disableCdc(schema, ks, table);
}));Read that we were removing cdcEnabledTables from our schema.
Renaming this variable to remainingCdcEnabledTables or schemaCdcEnabledTables plus a comment would help disambiguate this so it's clear we're not unregistering cdc enabled tables there at the end of the loop. The logic looks right but this naming threw me for a loop.
| } | ||
| } | ||
|
|
||
| private static void unregisterTable(@NotNull Schema schema, @NotNull String keyspace, @NotNull String table) |
There was a problem hiding this comment.
I think we should rename this. CassandraSchema#unregisterTable implies it's for just straight "unregistered your tables", vs. a name like CassandraSchema#unregisterNonCdcTables, which is where we see the "Refusing to unregistered CDC-enabled table" log message making a lot more sense.
…bled table drops the CDC table's mutation
a37972b to
5850660
Compare
jmckenzie-dev
left a comment
jmckenzie-dev
left a comment
There was a problem hiding this comment.
Just some comment changes remaining but otherwise LGTM
jmckenzie-dev
left a comment
jmckenzie-dev
left a comment
There was a problem hiding this comment.
Looks good. Thanks!
2 participants
No description provided.