yifan-c commented on code in PR #94:
URL: https://github.com/apache/cassandra-sidecar/pull/94#discussion_r1467959946
##########
src/main/java/org/apache/cassandra/sidecar/utils/SSTableUploader.java:
##########
@@ -76,14 +78,14 @@ public SSTableUploader(Vertx vertx,
* @param readStream server request from which file upload is
acquired
* @param uploadDirectory the absolute path to the upload directory in
the target {@code fs}
* @param componentFileName the file name of the component
- * @param expectedChecksum for verifying upload integrity, passed in
through request
+ * @param headers request headers
* @param filePermissions specifies the posix file permissions used to
create the SSTable file
* @return path of SSTable component to which data was uploaded
*/
public Future<String> uploadComponent(ReadStream<Buffer> readStream,
String uploadDirectory,
String componentFileName,
- String expectedChecksum,
+ MultiMap headers,
Review Comment:
Passing http request headers to this layer feels odd to me.
Request handling should be done in the corresponding handler, e.g.
extracting values from header and convert them into a digest object.
The uploader should care about its own responsibility only, which is
upload/store files on local disk.
(Ideally, checksum verification should be a dedicated step, but it has been
there already, so no need to change)
##########
common/src/main/java/org/apache/cassandra/sidecar/common/data/Digest.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * 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.common.data;
+
+import java.util.Map;
+import java.util.function.Consumer;
+
+/**
+ * Interface that represents a checksum digest
+ */
+public interface Digest extends Consumer<Map<String, String>>
Review Comment:
- The `Digest` interface is only used from the client side to put headers.
Can you relocate the interface and its implementations to client package?
- Despite `Consumer` does the job seemingly, can we have a more dedicated
interface for digest?
```java
Digest
{
// supply the header to be used in http request
Map<String, String> headers();
}
// usage, e.g. in UploadSSTableRequest#headers() method, merge the headers
with others
Map<String, String> headers = new HashMap<>(super.headers());
headers.putAll(digest.headers());
```
##########
src/main/java/org/apache/cassandra/sidecar/utils/ChecksumVerifier.java:
##########
@@ -30,9 +31,19 @@
public interface ChecksumVerifier
{
/**
- * @param checksum expected checksum value
- * @param filePath path to SSTable component
- * @return String component path, if verification is a success, else a
failed future is returned
+ * @param options a map with options required for validation
+ * @param filePath path to SSTable component
+ * @return a future String with the component path if verification is a
success, otherwise a failed future
*/
- Future<String> verify(String checksum, String filePath);
+ Future<String> verify(MultiMap options, String filePath);
Review Comment:
The checksum verifier should not be responsible for parsing headers for the
same reason mentioned in the other comment.
I think you are missing a server-side `Digest` data structure, which
supplies the input for verification.
One approach is this.
```java
public interface ChecksumVerifier<D extends Digest>
{
Future<String> verify(D digest, String filePath);
default boolean canVerify(D digest)
{
return false;
}
}
// And then,
public class MD5ChecksumVerifier extends AsyncFileChecksumVerifier<MD5Digest>
```
Beside that, as a nit, I would rename `ChecksumVerifier` to `DigestVerifier`
on server code for consistency and accuracy.
--
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]
