You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by borisroman <gi...@git.apache.org> on 2015/08/03 17:11:47 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-8703: Fixed issue when listing...
GitHub user borisroman opened a pull request:
https://github.com/apache/cloudstack/pull/651
CLOUDSTACK-8703: Fixed issue when listing directory on S3.
It would only return objectSummaries when the anwser from the S3 System was truncated.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/borisroman/cloudstack CLOUDSTACK-8703
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/651.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #651
----
commit 5f87e9c917faa3f7d1d8b8d3f73f569e83d7bc2d
Author: Boris Schrijver <bo...@pcextreme.nl>
Date: 2015-08-03T15:10:05Z
CLOUDSTACK-8703: Fixed issue when listing directory on S3, it would only return objectSummaries when the anwser from the S3 System was truncated.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8703: Fixed issue when listing...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/651#issuecomment-127376598
@remibergsma I will look at this with @borisroman tomorrow at the office. I want to verify his logic. Code seems good for now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8703: Fixed issue when listing...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/651#issuecomment-127312399
@borisroman @remibergsma the timeout is not related to anything but travis and maybe to the way we use it
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8703: Fixed issue when listing...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/651#issuecomment-127366917
I cannot test the functionality myself right now, although it looks OK. @wido can you verify and give a final LGTM? I'll then merge it.
@borisroman my only comment is your commit message. Please use 50 chars or less on the first line (as a title), then an empty line and finally any extra info or a description.
Example:
Fixed issue when listing directory on S3
CLOUDSTACK-8703: It would only return objectSummaries when the anwser from the S3 System was truncated.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8703: Fixed issue when listing...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/651#issuecomment-127375332
@DaanHoogland If it fits, its OK. Usually when people do this, it doesn't fit any more and GitHub will cut it. If the issue id is in the body, you can still search for it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8703: Fixed issue when listing...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/651#issuecomment-127292168
The failing check is a Travis timeout. Seems not related, is it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8703: Fixed issue when listing...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/651#issuecomment-127569273
Reviewed the code. LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8703: Fixed issue when listing...
Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/651#discussion_r36103914
--- Diff: utils/src/com/cloud/utils/S3Utils.java ---
@@ -352,10 +352,15 @@ public static File getFile(final ClientOptions clientOptions, final String bucke
ListObjectsRequest listObjectsRequest = new ListObjectsRequest().withBucketName(bucketName).withPrefix(directory + SEPARATOR);
ObjectListing ol = client.listObjects(listObjectsRequest);
- while (ol != null && ol.isTruncated()) {
+ if(ol.isTruncated()) {
--- End diff --
It is, it will thrown an exception not return null.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8703: Fixed issue when listing...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/651#issuecomment-127360713
thanks for the comments Boris LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8703: Fixed issue when listing...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/651
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8703: Fixed issue when listing...
Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/651#discussion_r36103980
--- Diff: utils/src/com/cloud/utils/S3Utils.java ---
@@ -352,10 +352,15 @@ public static File getFile(final ClientOptions clientOptions, final String bucke
ListObjectsRequest listObjectsRequest = new ListObjectsRequest().withBucketName(bucketName).withPrefix(directory + SEPARATOR);
ObjectListing ol = client.listObjects(listObjectsRequest);
--- End diff --
Will improve it when I update the entire S3 implementation to the latest AWS sdk version, will be in the next month.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8703: Fixed issue when listing...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/651#discussion_r36102819
--- Diff: utils/src/com/cloud/utils/S3Utils.java ---
@@ -352,10 +352,15 @@ public static File getFile(final ClientOptions clientOptions, final String bucke
ListObjectsRequest listObjectsRequest = new ListObjectsRequest().withBucketName(bucketName).withPrefix(directory + SEPARATOR);
ObjectListing ol = client.listObjects(listObjectsRequest);
--- End diff --
we don't catch any of the amazon runtime exceptions. I think we should wrap them in a CloudstackRuntimeException so they can be caught be the servlet before propagating into the container. This is not in your PR @borisroman so I won't -1 on it but if you are on it, why not improve ;)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8703: Fixed issue when listing...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/651#discussion_r36102628
--- Diff: utils/src/com/cloud/utils/S3Utils.java ---
@@ -352,10 +352,15 @@ public static File getFile(final ClientOptions clientOptions, final String bucke
ListObjectsRequest listObjectsRequest = new ListObjectsRequest().withBucketName(bucketName).withPrefix(directory + SEPARATOR);
ObjectListing ol = client.listObjects(listObjectsRequest);
- while (ol != null && ol.isTruncated()) {
+ if(ol.isTruncated()) {
--- End diff --
is ol guaranteed not to be null? I don't see any specific remark on this in the amazon javadoc.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8703: Fixed issue when listing...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/651#issuecomment-127368979
@remibergsma Are you sure you don't want the issue ref on the first line?
CLOUDSTACK-8703: Fixed issue when listing directory on S3
It would only return objectSummaries when the anwser from the S3 System was truncated.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---