yifan-c commented on code in PR #85:
URL: https://github.com/apache/cassandra-sidecar/pull/85#discussion_r1431780535
##########
adapters/base/src/main/java/org/apache/cassandra/sidecar/adapters/base/CassandraStorageOperations.java:
##########
@@ -40,7 +40,7 @@
import static
org.apache.cassandra.sidecar.adapters.base.StorageJmxOperations.STORAGE_SERVICE_OBJ_NAME;
/**
- * An implementation of the {@link StorageOperations} that interfaces with
Cassandra 4.0 and later
+ * An implementation of the {@link StorageOperations} that interfaces with
Cassandra 4.1 and later
Review Comment:
The comment feels misleading that the base is for cassandra 4.1+, but the
subclass (`Cassandra40StorageOperations`) is for cassandra 4.0+. Can we revise
the comment in the base class? I think it is not bound to specific version.
##########
adapters/base/src/main/java/org/apache/cassandra/sidecar/adapters/base/CassandraAdapter.java:
##########
@@ -41,17 +41,17 @@
import org.jetbrains.annotations.Nullable;
/**
- * A {@link ICassandraAdapter} implementation for Cassandra 4.0 and later
+ * A {@link ICassandraAdapter} implementation for Cassandra 4.1 and later
*/
public class CassandraAdapter implements ICassandraAdapter
{
private static final Logger LOGGER =
LoggerFactory.getLogger(CassandraAdapter.class);
protected final DnsResolver dnsResolver;
protected final JmxClient jmxClient;
- private final CQLSessionProvider cqlSessionProvider;
- private final InetSocketAddress localNativeTransportAddress;
- private final DriverUtils driverUtils;
- private volatile Host host;
+ protected final CQLSessionProvider cqlSessionProvider;
+ protected final InetSocketAddress localNativeTransportAddress;
+ protected final DriverUtils driverUtils;
+ protected volatile Host host;
Review Comment:
hmm... exposing a volatile field to be accessible from subclasses is prone
to race error.
##########
adapters/cassandra40/src/main/java/org/apache/cassandra/sidecar/adapters/cassandra40/Cassandra40Adapter.java:
##########
@@ -0,0 +1,53 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.sidecar.adapters.cassandra40;
+
+import java.net.InetSocketAddress;
+
+import org.apache.cassandra.sidecar.adapters.base.CassandraAdapter;
+import org.apache.cassandra.sidecar.common.CQLSessionProvider;
+import org.apache.cassandra.sidecar.common.ICassandraAdapter;
+import org.apache.cassandra.sidecar.common.JmxClient;
+import org.apache.cassandra.sidecar.common.StorageOperations;
+import org.apache.cassandra.sidecar.common.dns.DnsResolver;
+import org.apache.cassandra.sidecar.common.utils.DriverUtils;
+
+/**
+ * A {@link ICassandraAdapter} implementation for Cassandra 4.0 and later
+ */
+public class Cassandra40Adapter extends CassandraAdapter
Review Comment:
Why create `cassandra40`, instead of creating `cassandra41` and keep the
base as 4.0+?
##########
client/src/main/java/org/apache/cassandra/sidecar/client/request/SnapshotRequest.java:
##########
@@ -29,26 +30,38 @@ abstract class SnapshotRequest<T> extends
DecodableRequest<T>
{
SnapshotRequest(String keyspace, String table, String snapshotName)
{
- super(requestURI(keyspace, table, snapshotName, false));
+ super(requestURI(keyspace, table, snapshotName, false, null));
}
- SnapshotRequest(String keyspace, String table, String snapshotName,
boolean includeSecondaryIndexFiles)
+ SnapshotRequest(String keyspace, String table, String snapshotName,
boolean includeSecondaryIndexFiles,
+ @Nullable String snapshotTTL)
{
- super(requestURI(keyspace, table, snapshotName,
includeSecondaryIndexFiles));
+ super(requestURI(keyspace, table, snapshotName,
includeSecondaryIndexFiles, snapshotTTL));
}
- static String requestURI(String keyspace, String tableName, String
snapshotName, boolean includeSecondaryIndexFiles)
+ static String requestURI(String keyspace, String tableName, String
snapshotName,
+ boolean includeSecondaryIndexFiles, @Nullable
String snapshotTTL)
{
String requestUri = ApiEndpointsV1.SNAPSHOTS_ROUTE
.replaceAll(ApiEndpointsV1.KEYSPACE_PATH_PARAM,
keyspace)
.replaceAll(ApiEndpointsV1.TABLE_PATH_PARAM,
tableName)
.replaceAll(ApiEndpointsV1.SNAPSHOT_PATH_PARAM,
snapshotName);
- if (!includeSecondaryIndexFiles)
+ if (!includeSecondaryIndexFiles && snapshotTTL == null)
{
return requestUri;
}
- return requestUri + "?includeSecondaryIndexFiles=true";
+ if (!includeSecondaryIndexFiles)
+ {
+ return requestUri + "?ttl=" + snapshotTTL;
+ }
+
+ if (snapshotTTL == null)
+ {
+ return requestUri + "?includeSecondaryIndexFiles=true";
+ }
Review Comment:
The logic here is a bit hard to read, since the condition does not relate
well with the statement inside.
I would have this.
```java
if (!includeSecondaryIndexFiles && snapshotTTL == null)
{
return requestUri;
}
requestUri = requestUri + '?';
if (includeSecondaryIndexFiles)
{
requestUri = requestUri + "includeSecondaryIndexFiles=true";
}
if (snapshotTTL != null)
{
requestUri = requestUri + "?ttl=" + snapshotTTL;
}
return requestUri;
```
--
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]
