Trace Cassandra Tables' Metadata - #1657
Conversation
…data-cassandra-replacement
…data-cassandra-replacement
…data-cassandra-replacement
|
|
||
| public CassandraTableMetadata(String keyspaceName, String tableName, List<CassandraColumnMetadata> columns) { | ||
| this.keyspaceName = keyspaceName; | ||
| this.tableName = tableName; |
There was a problem hiding this comment.
The keyspaceName might be null, but the tableName can't. Queries with a null tableName would not have their schema traced due to the condition in line 81 of CqlSessionClassReplacement
There was a problem hiding this comment.
therefore a Objects.requireNull for tableName is required, right?
| this.keyspaceName = keyspaceName; | ||
| this.tableName = tableName; | ||
| this.columns = Collections.unmodifiableList(columns); | ||
| } |
There was a problem hiding this comment.
should you check if column names are repeated?
There was a problem hiding this comment.
There should be no duplicate column names in a table
There was a problem hiding this comment.
Shouldn't you double check that when the list is copied?
| return Objects.hash(keyspaceName, tableName, asSet(columns)); | ||
| } | ||
|
|
||
| private static Set<CassandraColumnMetadata> asSet(List<CassandraColumnMetadata> columns) { |
There was a problem hiding this comment.
this shouldn't be a method, right?
| private static TableReference extractTableReference(String query) { | ||
| if (query == null) { | ||
| return new TableReference(null, null); | ||
| return new TableReference(null, null, null, null); |
There was a problem hiding this comment.
if the query is not supported it should throw an IllegalArgumentException, right?
|
|
||
| public CassandraTableMetadata(String keyspaceName, String tableName, List<CassandraColumnMetadata> columns) { | ||
| this.keyspaceName = keyspaceName; | ||
| this.tableName = tableName; |
There was a problem hiding this comment.
therefore a Objects.requireNull for tableName is required, right?
| this.keyspaceName = keyspaceName; | ||
| this.tableName = tableName; | ||
| this.columns = Collections.unmodifiableList(columns); | ||
| } |
There was a problem hiding this comment.
Shouldn't you double check that when the list is copied?
| private static TableReference extractTableReference(String query) { | ||
| if (query == null) { | ||
| return new TableReference(null, null); | ||
| throw new IllegalArgumentException("query cannot be null"); |
There was a problem hiding this comment.
use Objects.requireNonNull instead of if...
| Matcher matcher = TABLE_REFERENCE_PATTERN.matcher(query); | ||
| if (!matcher.find()) { | ||
| return new TableReference(null, null); | ||
| return new TableReference(null, null, null, null); |
There was a problem hiding this comment.
what is a TableReference(null, null, null, null) ? shouldn't this return ``null'' instead of this reference? If this a special sentinel value, then a constant should be used.
| try { | ||
| CassandraSchemaTracer.resolve(cqlSession, ref.rawKeyspaceName, ref.rawTableName); | ||
| } catch (RuntimeException e) { | ||
| SimpleLogger.uniqueWarn("Failed to capture Cassandra schema for table " + ref.rawTableName); |
There was a problem hiding this comment.
How expected is this exception? If this is not expected at all exception should be thrown.
2 participants
No description provided.