Repository navigation
Conversation
| String fullSchemaText = DriverUnsupportedSchemaCache.concatSchemas(databaseAccessor.fullSchema(), | ||
| driverUnsupportedSchemaCache.getFullSchema()); | ||
| String fullSchemaText = schemaCache.concatSchemas(databaseAccessor.fullSchema(), | ||
| schemaCache.getFullSchema()); |
There was a problem hiding this comment.
I think this now duplicates the whole schema. getFullSchema() used to return only the tables the driver couldn't parse, but it now returns the full DESCRIBE output. Combining it with databaseAccessor.fullSchema() means every keyspace and table reaches CdcSchemaUtils.buildTablesToRegisterForCDC twice, in two formats. Should this call getUnsupportedTableSchema() instead?
The unit test doesn't catch this because it mocks getFullSchema() to return "". It would be good to update the mock so the test exercises the real combination.
Small nit: concatSchemas is static, so SchemaCache.concatSchemas(...) would read more clearly than calling it through the instance.
| { | ||
| fullSchema = assembleFullSchema(keyspaceSchemaCache); | ||
| fullSchemaCache.compareAndSet(null, fullSchema); | ||
| } |
There was a problem hiding this comment.
I think there's a small race here. Thread A sees fullSchemaCache == null and starts building from keyspaceSchemaCache. Meanwhile thread B calls getKeyspaceSchema("new_ks"), adds it to the map and calls fullSchemaCache.set(null). Thread A's compareAndSet(null, …) then still succeeds, with a schema that doesn't include new_ks. That stale value is served until the next refresh, which contradicts the javadoc saying keyspaces looked up individually are included.
A version or stamp check would close this gap.
| { | ||
| throw schemaNotAvailable(); | ||
| } | ||
| return new SchemaResponse(schema); |
There was a problem hiding this comment.
The per-keyspace endpoint upper-cases WITH replication to WITH REPLICATION, but the full-schema endpoint returns the raw DESCRIBE output, so the two endpoints are now inconsistent. The full schema also now includes the virtual keyspaces (system_views, system_virtual_schema) as comment blocks, which the driver output never had. Is that intended?
| context.json(schemaResponse); | ||
| return; | ||
| } | ||
| return keyspaceSchema.replaceFirst("WITH replication", "WITH REPLICATION"); |
There was a problem hiding this comment.
Replacing this one keyword feels fragile. DESCRIBE output differs from the driver's exportAsString in other ways too (indentation and line breaks, option ordering, durable_writes, how UDTs and indexes are written), so clients parsing the old format may break regardless. Could we normalize the output once in SchemaCache, so all consumers (both endpoints and the CDC monitor) see the same format? If the new format is intended, it'd be worth documenting the change in CHANGES.txt or the API docs instead.
| private ParameterizedClassConfiguration authProvider; | ||
| private SslConfiguration sslConfiguration; | ||
| private SecondBoundConfiguration unsupportedTableSchemaRefreshTime = DEFAULT_UNSUPPORTED_TABLE_SCHEMA_REFRESH_TIME; | ||
| private SecondBoundConfiguration unsupportedTableSchemaRefreshTime; |
There was a problem hiding this comment.
With the builder default removed, the deprecated but still public unsupportedTableSchemaRefreshTime() returns null instead of 5m when the property isn't set. Any remaining caller that uses the value would hit an NPE. Could we keep the 5m default until the method is actually removed?
| } | ||
| String schema = String.join(STATEMENT_DELIMITER, cqlSchema); | ||
| cache.put(keyspace, schema); | ||
| fullSchemaCache.set(null); |
There was a problem hiding this comment.
fullSchemaCache.set(null) runs on every call, including during refresh() while we're filling a new map that hasn't been published yet. That clears the live cache N times per refresh, and any concurrent full-schema request rebuilds it from the old map each time. Could we only clear it when writing to the live keyspaceSchemaCache, or once after the new map is swapped in?
2 participants
No description provided.