yifan-c commented on code in PR #70:
URL: https://github.com/apache/cassandra-sidecar/pull/70#discussion_r1355461793
##########
common/src/main/java/org/apache/cassandra/sidecar/common/ApiEndpointsV1.java:
##########
@@ -55,6 +57,8 @@ public final class ApiEndpointsV1
public static final String SNAPSHOTS_ROUTE = API_V1 + PER_KEYSPACE +
PER_TABLE + PER_SNAPSHOT;
// Replaces DEPRECATED_COMPONENTS_ROUTE
public static final String COMPONENTS_ROUTE = SNAPSHOTS_ROUTE +
PER_COMPONENT;
+ public static final String COMPONENTS_WITH_INDEX_ROUTE_SUPPORT =
SNAPSHOTS_ROUTE + PER_INDEX
Review Comment:
To be more clear, the name should indicate that it is for secondary index.
The name `index` has been used else where in the other classes too. Please
update them.
Suggestions: `COMPONENTS_WITH_SI_ROUTE_SUPPORT` or
`COMPONENTS_WITH_SECONDARY_INDEX_ROUTE_SUPPORT`
I think using `SI` is fine, as it is a common term in Cassandra world.
##########
src/main/java/org/apache/cassandra/sidecar/data/StreamSSTableComponentRequest.java:
##########
@@ -43,24 +45,74 @@ public class StreamSSTableComponentRequest extends
SSTableComponent
@VisibleForTesting
public StreamSSTableComponentRequest(String keyspace, String tableName,
String snapshotName, String componentName)
{
- this(new QualifiedTableName(keyspace, tableName, true), snapshotName,
componentName);
+ this(new QualifiedTableName(keyspace, tableName, true), snapshotName,
null, componentName);
+ }
+
+ /**
+ * Constructor for the holder class
+ *
+ * @param keyspace the keyspace in Cassandra
+ * @param tableName the table name in Cassandra
+ * @param snapshotName the name of the snapshot
+ * @param indexName the name of the index for the SSTable component
+ * @param componentName the name of the SSTable component
+ */
+ @VisibleForTesting
+ public StreamSSTableComponentRequest(String keyspace, String tableName,
String snapshotName, String indexName,
+ String componentName)
+ {
+ this(new QualifiedTableName(keyspace, tableName, true), snapshotName,
indexName, componentName);
+ }
+
+ /**
+ * Constructor for the holder class
+ *
+ * @param qualifiedTableName the qualified table name in Cassandra
+ * @param snapshotName the name of the snapshot
+ * @param componentName the name of the SSTable component
+ */
+ public StreamSSTableComponentRequest(QualifiedTableName qualifiedTableName,
+ String snapshotName,
+ String componentName)
+ {
+ this(qualifiedTableName, snapshotName, null, componentName);
}
/**
* Constructor for the holder class
*
* @param qualifiedTableName the qualified table name in Cassandra
* @param snapshotName the name of the snapshot
+ * @param indexName the name of the index for the SSTable
component
* @param componentName the name of the SSTable component
*/
public StreamSSTableComponentRequest(QualifiedTableName qualifiedTableName,
String snapshotName,
+ @Nullable String indexName,
String componentName)
{
- super(qualifiedTableName, componentName);
+ super(qualifiedTableName, indexName, componentName);
this.snapshotName = Objects.requireNonNull(snapshotName, "snapshotName
must not be null");
}
+ public static StreamSSTableComponentRequest from(QualifiedTableName
qualifiedTableName, RoutingContext context)
+ {
+ String snapshotName = context.pathParam("snapshot");
+ String indexName = context.pathParam("index");
+ String componentName = context.pathParam("component");
+
+ if (indexName == null)
+ {
+ return new StreamSSTableComponentRequest(qualifiedTableName,
+ snapshotName,
+ componentName);
+ }
Review Comment:
The nullness check seems unnecessary. The `StreamSSTableComponentRequest`
allows taking null value.
##########
common/src/main/java/org/apache/cassandra/sidecar/common/data/SSTableComponent.java:
##########
@@ -35,8 +39,21 @@ public class SSTableComponent
* @param componentName the name of the SSTable component
*/
public SSTableComponent(QualifiedTableName qualifiedTableName, String
componentName)
+ {
+ this(qualifiedTableName, null, componentName);
+ }
+
+ /**
+ * Constructor for the holder class
+ *
+ * @param qualifiedTableName the qualified table name in Cassandra
+ * @param indexName the name of the index for the SSTable
component
+ * @param componentName the name of the SSTable component
+ */
+ public SSTableComponent(QualifiedTableName qualifiedTableName, @Nullable
String indexName, String componentName)
Review Comment:
Is it possible to just modify the existing constructor? Let's try to not add
more code.
##########
src/main/java/org/apache/cassandra/sidecar/data/StreamSSTableComponentRequest.java:
##########
@@ -43,24 +45,74 @@ public class StreamSSTableComponentRequest extends
SSTableComponent
@VisibleForTesting
public StreamSSTableComponentRequest(String keyspace, String tableName,
String snapshotName, String componentName)
{
- this(new QualifiedTableName(keyspace, tableName, true), snapshotName,
componentName);
+ this(new QualifiedTableName(keyspace, tableName, true), snapshotName,
null, componentName);
+ }
+
+ /**
+ * Constructor for the holder class
+ *
+ * @param keyspace the keyspace in Cassandra
+ * @param tableName the table name in Cassandra
+ * @param snapshotName the name of the snapshot
+ * @param indexName the name of the index for the SSTable component
+ * @param componentName the name of the SSTable component
+ */
+ @VisibleForTesting
+ public StreamSSTableComponentRequest(String keyspace, String tableName,
String snapshotName, String indexName,
+ String componentName)
+ {
+ this(new QualifiedTableName(keyspace, tableName, true), snapshotName,
indexName, componentName);
+ }
+
+ /**
+ * Constructor for the holder class
+ *
+ * @param qualifiedTableName the qualified table name in Cassandra
+ * @param snapshotName the name of the snapshot
+ * @param componentName the name of the SSTable component
+ */
+ public StreamSSTableComponentRequest(QualifiedTableName qualifiedTableName,
+ String snapshotName,
+ String componentName)
+ {
+ this(qualifiedTableName, snapshotName, null, componentName);
Review Comment:
Similar to the above comment. How about just modify the constructor? The
class is for server internal usage, not a public API.
##########
src/main/java/org/apache/cassandra/sidecar/snapshots/SnapshotPathBuilder.java:
##########
@@ -283,6 +287,13 @@ protected void validate(StreamSSTableComponentRequest
request)
validator.validateTableName(request.tableName());
validator.validateSnapshotName(request.snapshotName());
// Only allow .db and TOC.txt components here
+ String indexName = request.indexName();
Review Comment:
secondaryIndexName or siName.
Not to be confused with primary index, which is an sstable component.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
