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 1281- DLO Swift Implementation (#1090)
Date Sat, 29 Apr 2017 19:54:47 GMT
andrewgaul requested changes on this pull request.

Please address substantial comments and remove all stylistic changes.  Also investigate these
failed tests:

```
  CloudFilesUSBlobIntegrationLiveTest>BaseBlobIntegrationTest.deleteObjectNoContainer:525
» NullPointer
  CloudFilesUSBlobIntegrationLiveTest>BaseBlobIntegrationTest.deleteObjectNotFound:466
» NullPointer
  CloudFilesUSBlobIntegrationParallelLiveTest>RegionScopedSwiftBlobStoreParallelLiveTest.downloadParallelBlob:177
expected [bf9fdd871a0ba46a93f970aed60d9058] but found [813d97604fbcd3961dd2be9a99f9ed41]
  CloudFilesUSBlobIntegrationParallelLiveTest>RegionScopedSwiftBlobStoreParallelLiveTest.streamParallelBlob:194
expected [bf9fdd871a0ba46a93f970aed60d9058] but found [813d97604fbcd3961dd2be9a99f9ed41]
```

>        }
-      return api.getContainerApi(regionId).create(container, BASIC_CONTAINER);
+      resultContainerCreate = api.getContainerApi(regionId).create(container, BASIC_CONTAINER);
+      if (resultContainerCreate) {
+         Container val = api.getContainerApi(regionId).get(container);
+         containerCache.put(container, Optional.fromNullable(val));
+      }
+      return resultContainerCreate;

How is creating a container related to dynamic large objects?  If this is a separate change,
submit a separate pull request.

> @@ -17,6 +17,7 @@
 package org.jclouds.openstack.swift.v1;
 
 import java.io.Closeable;
+

Please sweep through the entire pull request and remove these whitespace changes.

>        // deleting a object only deletes the manifest, leaving the subobjects.
       // We first try a multipart delete and if that fails since the object is
       // not an MPU we fall back to single-part delete.
-      DeleteStaticLargeObjectResponse response = api.getStaticLargeObjectApi(regionId, container).delete(name);
-      if (!response.status().equals("200 OK")) {
+
+      String objManifest = getManifestInfo(container, name);
+      String sloInfo = getSLOInfo(container, name);

Previously this code issued one or two DELETEs depending if the name was an SLO or a regular
object.  With this commit we issue always issue three RPCs, getWithoutBody, getWithoutBody,
and delete with some flags.  How can you optimize this?  Also this fails if the object does
not exist.

>        return api.getStaticLargeObjectApi(regionId, mpu.containerName()).replaceManifest(mpu.blobName(),
             builder.build(), mpu.blobMetadata().getUserMetadata(), getContentMetadataForManifest(mpu.blobMetadata().getContentMetadata()));
    }
+   
+   public String completeMultipartUploadDLO(MultipartUpload mpu, List<MultipartPart>
parts) {

Remove - we should not expose dynamic large objects in the portable abstraction!  Users can
call the provider-level APIs for this functionality.

> +      }
+      return null;
+   }
+   
+   /**
+    * Gets X-Static-Large-Object for SLO uploaded file
+    * @param container
+    * @param objectName
+    * @return true if object is SLO uploaded else null
+    */
+   private String getSLOInfo(String container, String objectName) {
+      ObjectApi objectApi = api.getObjectApi(regionId, container);
+      Multimap<String, String> objInfo = objectApi.getWithoutBody(objectName).getHeaders();
+      if (objInfo.containsKey("X-Static-Large-Object")) {
+         Collection<String> objectManifest = objInfo.get("X-Static-Large-Object");
+         return String.valueOf(objectManifest.toArray()[0]);

Consider the simpler:

```java
ObjectApi objectApi = api.getObjectApi(regionId, container);
Multimap<String, String> objInfo = objectApi.getWithoutBody(objectName).getHeaders();
Collection<String> objectManifest = objInfo.get("X-Static-Large-Object");
return Iterables.getFirst(objectManifest, null);
```

> +    */
+   private String getSLOInfo(String container, String objectName) {
+      ObjectApi objectApi = api.getObjectApi(regionId, container);
+      Multimap<String, String> objInfo = objectApi.getWithoutBody(objectName).getHeaders();
+      if (objInfo.containsKey("X-Static-Large-Object")) {
+         Collection<String> objectManifest = objInfo.get("X-Static-Large-Object");
+         return String.valueOf(objectManifest.toArray()[0]);
+      }
+      return null;
+   }
+   
+   
+   /**
+    * @param containerAndPrefix
+    */
+   private void removeObjectsWithPrefix(String containerAndPrefix) {

Add `static`.

> +    * @param containerAndKey
+    * @return array of size 2 with container and objectPrefix
+    */
+   private String[] splitContainerAndKey(String containerAndKey) {
+      String[] parts = containerAndKey.split("/", 2);
+      checkArgument(parts.length == 2, "No / separator found in \"%s\"", containerAndKey);
+      return parts;
+   }
+    
+   /**
+    * Retrieves a list of existing storage
+    * @param container
+    * @param options
+    * @return maximum of 10,000 object names per request.
+    */
+   private PageSet<? extends StorageMetadata> list(final String container,

Why do you reimplement `ObjectApi.list`?

> + * 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.jclouds.openstack.swift.v1.options;
+
+import com.google.common.util.concurrent.ListeningExecutorService;
+import org.jclouds.blobstore.options.PutOptions;
+
+public class OpenStackSwiftPutOptions extends PutOptions {

Remove this entire thing and make users call either the dynamic or static APIs.

> +            getApi().getObjectApi(regionId, containerName).delete(name);
+      }
+   }
+
+   @Override
+   @BeforeClass(groups = "live")
+   public void setup() {
+      super.setup();
+      for (String regionId : regions) {
+         boolean created = getApi().getContainerApi(regionId).create(defaultContainerName);
+         if (!created) {
+            deleteAllObjectsInContainer(regionId, defaultContainerName);
+         }
+      }
+
+      megOf1s = new byte[1024 * 1024];

Call `TestUtils.randomByteSource()` instead.

>  
    // TODO: This exposes ListeningExecutorService to the user, instead of a regular ExecutorService
-   private ListeningExecutorService customExecutor = MoreExecutors.sameThreadExecutor();
+   protected ListeningExecutorService customExecutor = MoreExecutors.sameThreadExecutor();

Revert this after removing `OpenStackSwiftPutOptions`.

-- 
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/1090#pullrequestreview-35503143
Mime
View raw message