jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Gaul <notificati...@github.com>
Subject Re: [jclouds/jclouds] JCLOUDS-1450: Use S3-style ETags for MPUs. (#1251)
Date Wed, 24 Oct 2018 01:36:14 GMT
gaul requested changes on this pull request.

This seems reasonable.  CI may fail for unrelated reasons so please run these locally which
I will also do before merging:

```
mvn verify -pl :filesystem
mvn integration-test -pl :filesystem -Plive
```

> @@ -115,6 +116,7 @@
    private static final String XATTR_USER_METADATA_PREFIX = "user.user-metadata.";
    private static final byte[] DIRECTORY_MD5 =
            Hashing.md5().hashBytes(new byte[0]).asBytes();
+   private static final String MPU_ETAG_FORMAT = "^\"[a-f0-9]{32}-\\d+\"$";

Prefer `format = Pattern.compile()` and `format.matcher().matches`.  Also are ^$ necessary
since you use `match` instead of `find`?

> @@ -488,23 +498,37 @@ public String putBlob(final String containerName, final Blob blob)
throws IOExce
       String tmpBlobName = blobKey + "-" + UUID.randomUUID();
       File tmpFile = getFileForBlobKey(containerName, tmpBlobName);
       Path tmpPath = tmpFile.toPath();
-      HashingInputStream his = null;
+      boolean isMpu = false;
+      if (blob.getMetadata() != null && blob.getMetadata().getETag() != null)
+         isMpu = blob.getMetadata().getETag().matches(MPU_ETAG_FORMAT);
+      InputStream inputStream = null;
+      byte [] eTag = null;

Remove space before `[]`.

>        try {
          Files.createParentDirs(tmpFile);
-         his = new HashingInputStream(Hashing.md5(), payload.openStream());
-         long actualSize = Files.asByteSink(tmpFile).writeFrom(his);
+         if (isMpu) {
+            inputStream = payload.openStream();
+            eTag = blob.getMetadata().getETag().getBytes();
+         }
+         else {

Put `} else {` on a single line.

>        }
+      StringBuilder mpuETagBuilder = new StringBuilder("\"");
+      mpuETagBuilder.append(Hashing.md5().hashString(partHashes.toString(), US_ASCII).toString());
+      mpuETagBuilder.append(String.format("-%d\"", parts.size()));
+      String mpuETag = mpuETagBuilder.toString();

Use the return values from `StringBuilder` and avoid the `String.format` call:

```java
String mpuETag = new StringBuilder("\"")
    .append(Hashing.md5().hashString(partHashes.toString(), US_ASCII).toString())
    .append("-")
    .append(parts.size())
    .toString();
```

Repeated above.

> @@ -373,7 +376,13 @@ public Blob getBlob(final String container, final String key) {
                if (attributes.contains(XATTR_CONTENT_MD5)) {
                   ByteBuffer buf = ByteBuffer.allocate(view.size(XATTR_CONTENT_MD5));
                   view.read(XATTR_CONTENT_MD5, buf);
-                  hashCode = HashCode.fromBytes(buf.array());
+                  byte [] etagBytes = buf.array();
+                  if (etagBytes.length == 16) {
+                     hashCode = HashCode.fromBytes(buf.array());
+                     eTag = String.format("\"%s\"", hashCode.toString());

`String.format` will actually call `toString` for you, although this may read better:

```java
eTag = "\"" + hashCode + "\"";
```

> @@ -373,7 +376,13 @@ public Blob getBlob(final String container, final String key) {
                if (attributes.contains(XATTR_CONTENT_MD5)) {
                   ByteBuffer buf = ByteBuffer.allocate(view.size(XATTR_CONTENT_MD5));
                   view.read(XATTR_CONTENT_MD5, buf);
-                  hashCode = HashCode.fromBytes(buf.array());
+                  byte [] etagBytes = buf.array();
+                  if (etagBytes.length == 16) {
+                     hashCode = HashCode.fromBytes(buf.array());
+                     eTag = String.format("\"%s\"", hashCode.toString());
+                  } else {
+                     eTag = new String(etagBytes);

Should this have a second `Charset` parameter?

> @@ -878,7 +888,7 @@ public String completeMultipartUpload(MultipartUpload mpu, List<MultipartPart>
p
 
       setBlobAccess(mpu.containerName(), mpu.blobName(), mpu.putOptions().getBlobAccess());
 
-      return eTag;
+      return mpuETag;

Does this return an MPU ETag for single-part uploads?

> @@ -373,7 +376,13 @@ public Blob getBlob(final String container, final String key) {
                if (attributes.contains(XATTR_CONTENT_MD5)) {
                   ByteBuffer buf = ByteBuffer.allocate(view.size(XATTR_CONTENT_MD5));
                   view.read(XATTR_CONTENT_MD5, buf);
-                  hashCode = HashCode.fromBytes(buf.array());
+                  byte [] etagBytes = buf.array();
+                  if (etagBytes.length == 16) {

Add comments:

```
// single-part upload
// multi-part upload
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1251#pullrequestreview-167697206
Mime
View raw message