You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@jclouds.apache.org by jumenzel <no...@github.com> on 2019/01/15 15:41:17 UTC

[jclouds/jclouds] Adding support for AWS S3 get_objects V2 api call. (#1267)

- Adding support for AWS S3 get_objects V2 api call. 
You can view, comment on, or merge this pull request online at:

  https://github.com/jclouds/jclouds/pull/1267

-- Commit Summary --

  * Adding support for AWS S3 get_objects V2 api call.

-- File Changes --

    M apis/s3/src/main/java/org/jclouds/s3/blobstore/functions/ContainerToBucketListOptions.java (9)
    M apis/s3/src/main/java/org/jclouds/s3/options/ListBucketOptions.java (29)
    M apis/s3/src/main/java/org/jclouds/s3/reference/S3Constants.java (9)
    M apis/s3/src/main/java/org/jclouds/s3/xml/ListBucketHandler.java (4)
    M apis/s3/src/test/java/org/jclouds/s3/blobstore/functions/BucketToContainerListOptions.java (15)
    M blobstore/src/main/java/org/jclouds/blobstore/options/ListContainerOptions.java (19)

-- Patch Links --

https://github.com/jclouds/jclouds/pull/1267.patch
https://github.com/jclouds/jclouds/pull/1267.diff

-- 
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/1267

Re: [jclouds/jclouds] Adding support for AWS S3 get_objects V2 api call. (#1267)

Posted by Andrew Gaul <no...@github.com>.

> Just realized it's the integration test that is failing for filesystem.

Both of these succeed for me on Fedora 29:

```
mvn install -pl :filesystem
mvn integration-test -pl :filesystem
```

Probably better to file an issue with your symptoms since it is not related to this pull request.

-- 
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/1267#issuecomment-458427619

Re: [jclouds/jclouds] Adding support for AWS S3 get_objects V2 api call. (#1267)

Posted by jumenzel <no...@github.com>.

 $ mvn -version
```
Apache Maven 3.6.0 (97c98ec64a1fdfee7767ce5ffb20918da4f719f3; 2018-10-24T14:41:47-04:00)
Maven home: /usr/local/Cellar/maven/3.6.0/libexec
Java version: 1.8.0_181, vendor: Oracle Corporation, runtime: /Library/Java/JavaVirtualMachines/jdk1.8.0_181.jdk/Contents/Home/jre
Default locale: en_US, platform encoding: UTF-8
OS name: "mac os x", version: "10.14.2", arch: "x86_64", family: "mac"
```
It fails for the install goal. `mvn test -pl :filesystem` passes. 

-- 
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/1267#issuecomment-458388114

Re: [jclouds/jclouds] Adding support for AWS S3 get_objects V2 api call. (#1267)

Posted by jumenzel <no...@github.com>.

I'll make another attempt later this week. 

-- 
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/1267#issuecomment-472037075

Re: [jclouds/jclouds] Adding support for AWS S3 get_objects V2 api call. (#1267)

Posted by Andrew Gaul <no...@github.com>.

@jumenzel do you not plan to continue this pull request?  This feature is useful to return the owner information during list objects.

-- 
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/1267#issuecomment-469597947

Re: [jclouds/jclouds] Adding support for AWS S3 get_objects V2 api call. (#1267)

Posted by Andrew Gaul <no...@github.com>.

gaul requested changes on this pull request.



>  
    public ListContainerOptions() {
    }
 
    ListContainerOptions(Integer maxKeys, String marker, String dir, boolean recursive,
-            boolean detailed, String prefix, String delimiter) {
+            boolean detailed, String prefix, String delimiter, int listType) {

Sorry I missed this earlier, but why does the portable abstraction need to know the S3 list type?  This looks like a layering violation.

-- 
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/1267#pullrequestreview-197398732

Re: [jclouds/jclouds] Adding support for AWS S3 get_objects V2 api call. (#1267)

Posted by jumenzel <no...@github.com>.

Closed #1267.

-- 
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/1267#event-2174997326

Re: [jclouds/jclouds] Adding support for AWS S3 get_objects V2 api call. (#1267)

Posted by jumenzel <no...@github.com>.

I got everything done but the S3Test. I can't get the test I added to pass.
And I'm not able to run/debug the test from my IDE and I'm not able to get my IDE to debug via attach to process, it just won't stop at any break points. So I put this on hold for now.
I also noticed that without any of my changes building master fails: 
`[INFO] jclouds filesystem core ............................ FAILURE [ 19.118 s]`
The 2.1.x branch builds just fine. 

-- 
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/1267#issuecomment-458326857

Re: [jclouds/jclouds] Adding support for AWS S3 get_objects V2 api call. (#1267)

Posted by Andrew Gaul <no...@github.com>.

> I also noticed that without any of my changes building master fails:
> `[INFO] jclouds filesystem core ............................ FAILURE [ 19.118 s]`
> The 2.1.x branch builds just fine.

`mvn test -pl :filesystem` succeeds for me on Fedora 9 with Java 8.  Which environment do you build on?

-- 
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/1267#issuecomment-458328593

Re: [jclouds/jclouds] Adding support for AWS S3 get_objects V2 api call. (#1267)

Posted by jumenzel <no...@github.com>.

The difference that I found useful is the use of the ContinuationToken and NextContinuationToken for listing files in blocks. I think v2 also has a option to get some metadata of each object listed. 

-- 
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/1267#issuecomment-455333869

Re: [jclouds/jclouds] Adding support for AWS S3 get_objects V2 api call. (#1267)

Posted by Andrew Gaul <no...@github.com>.

gaul requested changes on this pull request.

Can you add tests to `S3Test`, `ListBucketOptionsTest`, and `BucketsLiveTest`?

> @@ -37,6 +37,15 @@
 public class ListBucketOptions extends BaseHttpRequestOptions implements Cloneable {
    public static final ListBucketOptions NONE = new ListBucketOptions();
 
+   public ListBucketOptions withListType(String listType) {

Should this be `int` to match other getters and setters?

> @@ -37,6 +37,15 @@
 public class ListBucketOptions extends BaseHttpRequestOptions implements Cloneable {
    public static final ListBucketOptions NONE = new ListBucketOptions();
 
+   public ListBucketOptions withListType(String listType) {
+      queryParameters.put("list-type", checkNotNull(listType, "listType"));
+      return this;
+   }
+
+   public String getListType() {

Similarly, should this be `Integer` (matching `getMaxResults`)?

> @@ -28,13 +28,24 @@
    public ListContainerOptions apply(ListBucketOptions[] optionsList) {
       ListContainerOptions options = new ListContainerOptions();
       if (optionsList.length != 0) {
+         int listType = 1;
+         if (optionsList[0].getListType() != null && optionsList[0].getListType().equals("2")) {

Prefer `Strings.nullToEmpty(optionsList[0].getListType()).equals("2")`.

-- 
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/1267#pullrequestreview-192727568

Re: [jclouds/jclouds] Adding support for AWS S3 get_objects V2 api call. (#1267)

Posted by jumenzel <no...@github.com>.

Just realized it's the the integration test that is failing for filesystem. 

-- 
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/1267#issuecomment-458388583

Re: [jclouds/jclouds] Adding support for AWS S3 get_objects V2 api call. (#1267)

Posted by Andrew Gaul <no...@github.com>.

Also what is the difference between v1 and v2 listing?

-- 
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/1267#issuecomment-454450710

Re: [jclouds/jclouds] Adding support for AWS S3 get_objects V2 api call. (#1267)

Posted by Andrew Gaul <no...@github.com>.

@jumenzel does this pull request need anything to move forward?

-- 
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/1267#issuecomment-458261834

Re: [jclouds/jclouds] Adding support for AWS S3 get_objects V2 api call. (#1267)

Posted by jumenzel <no...@github.com>.

jumenzel commented on this pull request.



> @@ -37,6 +37,15 @@
 public class ListBucketOptions extends BaseHttpRequestOptions implements Cloneable {
    public static final ListBucketOptions NONE = new ListBucketOptions();
 
+   public ListBucketOptions withListType(String listType) {
+      queryParameters.put("list-type", checkNotNull(listType, "listType"));
+      return this;
+   }
+
+   public String getListType() {

Again here the value returned would be either 2 or null. 

-- 
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/1267#discussion_r248839503

Re: [jclouds/jclouds] Adding support for AWS S3 get_objects V2 api call. (#1267)

Posted by jumenzel <no...@github.com>.

jumenzel commented on this pull request.



> @@ -37,6 +37,15 @@
 public class ListBucketOptions extends BaseHttpRequestOptions implements Cloneable {
    public static final ListBucketOptions NONE = new ListBucketOptions();
 
+   public ListBucketOptions withListType(String listType) {

It's a bit strange, the only value that's allowed is 2. That's how list objects v2 is triggered. 
With v1 you don't set list-type at all. So there's no list type v1. 
I can just make it an int and only allow value 2. 

-- 
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/1267#discussion_r248839439