You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by wido <gi...@git.apache.org> on 2015/07/14 09:18:07 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-8581: S3, make connection TTL ...
GitHub user wido opened a pull request:
https://github.com/apache/cloudstack/pull/582
CLOUDSTACK-8581: S3, make connection TTL and TCP KeepAlive configureable
Signed-off-by: Wido den Hollander <wi...@widodh.nl>
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/wido/cloudstack CLOUDSTACK-8581
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/582.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 #582
----
commit 2f1f4698b2da03805dacba89816b623d92f0fd2e
Author: Wido den Hollander <wi...@widodh.nl>
Date: 2015-07-14T06:17:49Z
CLOUDSTACK-8581: S3, make connection TTL and TCP KeepAlive configureable
Signed-off-by: Wido den Hollander <wi...@widodh.nl>
----
---
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-8581: S3, make connection TTL ...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/582#discussion_r34553635
--- Diff: plugins/storage/image/s3/src/org/apache/cloudstack/storage/datastore/driver/S3ImageStoreDriverImpl.java ---
@@ -60,7 +60,9 @@ public DataStoreTO getStoreTO(DataStore store) {
: Integer.valueOf(details.get(ApiConstants.S3_CONNECTION_TIMEOUT)), details.get(ApiConstants.S3_MAX_ERROR_RETRY) == null ? null
: Integer.valueOf(details.get(ApiConstants.S3_MAX_ERROR_RETRY)), details.get(ApiConstants.S3_SOCKET_TIMEOUT) == null ? null
: Integer.valueOf(details.get(ApiConstants.S3_SOCKET_TIMEOUT)), imgStore.getCreated(), _configDao.getValue(Config.S3EnableRRS.toString()) == null ? false
- : Boolean.parseBoolean(_configDao.getValue(Config.S3EnableRRS.toString())), getMaxSingleUploadSizeInBytes());
+ : Boolean.parseBoolean(_configDao.getValue(Config.S3EnableRRS.toString())), getMaxSingleUploadSizeInBytes(),
+ details.get(ApiConstants.S3_CONNECTION_TTL) == null ? null : Integer.valueOf(details.get(ApiConstants.S3_CONNECTION_TTL)),
+ details.get(ApiConstants.S3_USE_TCP_KEEPALIVE) == null ? null : Boolean.parseBoolean(details.get(ApiConstants.S3_USE_TCP_KEEPALIVE)));
--- End diff --
this is not so clear because of the formatting.
maybe put a parameter per line
and maybe put the ternary operator in a (set of) simple method calls?
---
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-8581: S3, make connection TTL ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/582
---
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-8581: S3, make connection TTL ...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/582#issuecomment-121191099
logic looks good with respect to the functional description. could do with tests
---
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-8581: S3, make connection TTL ...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/582#discussion_r34786110
--- Diff: api/src/com/cloud/agent/api/to/S3TO.java ---
@@ -118,6 +122,14 @@ public boolean equals(final Object thatObject) {
return false;
}
+ if (connectionTtl != null ? !connectionTtl.equals(thatS3TO.connectionTtl) : thatS3TO.connectionTtl != null) {
+ return false;
--- End diff --
I agree, but the other lines there also have the very complex if-else statements.
I want to write the S3 code anyway, it's buggy as hell
---
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-8581: S3, make connection TTL ...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/582#issuecomment-122029800
LGTM :+1:
Can I merge it, @wido, or would you like to do it?
Cheers,
Wilder
---
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-8581: S3, make connection TTL ...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/582#discussion_r34755599
--- Diff: api/src/com/cloud/agent/api/to/S3TO.java ---
@@ -118,6 +122,14 @@ public boolean equals(final Object thatObject) {
return false;
}
+ if (connectionTtl != null ? !connectionTtl.equals(thatS3TO.connectionTtl) : thatS3TO.connectionTtl != null) {
+ return false;
--- End diff --
LGTM, other than the complex ternary in the if().
---
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-8581: S3, make connection TTL ...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/582#discussion_r34674108
--- Diff: api/src/com/cloud/agent/api/to/S3TO.java ---
@@ -118,6 +122,14 @@ public boolean equals(final Object thatObject) {
return false;
}
+ if (connectionTtl != null ? !connectionTtl.equals(thatS3TO.connectionTtl) : thatS3TO.connectionTtl != null) {
+ return false;
--- End diff --
break into readable if-else please
---
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.
---
Re: [GitHub] cloudstack pull request: CLOUDSTACK-8581: S3, make
connection TTL ...
Posted by Daan Hoogland <da...@gmail.com>.
you can add that commit here for my part, please don't squash it;)
On Tue, Jul 14, 2015 at 12:30 PM, wido <gi...@git.apache.org> wrote:
> Github user wido commented on a diff in the pull request:
>
> https://github.com/apache/cloudstack/pull/582#discussion_r34554904
>
> --- Diff: plugins/storage/image/s3/src/org/apache/cloudstack/storage/datastore/driver/S3ImageStoreDriverImpl.java ---
> @@ -60,7 +60,9 @@ public DataStoreTO getStoreTO(DataStore store) {
> : Integer.valueOf(details.get(ApiConstants.S3_CONNECTION_TIMEOUT)), details.get(ApiConstants.S3_MAX_ERROR_RETRY) == null ? null
> : Integer.valueOf(details.get(ApiConstants.S3_MAX_ERROR_RETRY)), details.get(ApiConstants.S3_SOCKET_TIMEOUT) == null ? null
> : Integer.valueOf(details.get(ApiConstants.S3_SOCKET_TIMEOUT)), imgStore.getCreated(), _configDao.getValue(Config.S3EnableRRS.toString()) == null ? false
> - : Boolean.parseBoolean(_configDao.getValue(Config.S3EnableRRS.toString())), getMaxSingleUploadSizeInBytes());
> + : Boolean.parseBoolean(_configDao.getValue(Config.S3EnableRRS.toString())), getMaxSingleUploadSizeInBytes(),
> + details.get(ApiConstants.S3_CONNECTION_TTL) == null ? null : Integer.valueOf(details.get(ApiConstants.S3_CONNECTION_TTL)),
> + details.get(ApiConstants.S3_USE_TCP_KEEPALIVE) == null ? null : Boolean.parseBoolean(details.get(ApiConstants.S3_USE_TCP_KEEPALIVE)));
>
> --- End diff --
>
> Oh, I agree with you. It's just that I didn't want to touch the formatting for this patch.
>
> When I looked at the code I had the same idea.
>
> If this PR gets merged I want to send a second commit which fixes this.
>
>
> ---
> 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.
> ---
--
Daan
[GitHub] cloudstack pull request: CLOUDSTACK-8581: S3, make connection TTL ...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/582#discussion_r34554904
--- Diff: plugins/storage/image/s3/src/org/apache/cloudstack/storage/datastore/driver/S3ImageStoreDriverImpl.java ---
@@ -60,7 +60,9 @@ public DataStoreTO getStoreTO(DataStore store) {
: Integer.valueOf(details.get(ApiConstants.S3_CONNECTION_TIMEOUT)), details.get(ApiConstants.S3_MAX_ERROR_RETRY) == null ? null
: Integer.valueOf(details.get(ApiConstants.S3_MAX_ERROR_RETRY)), details.get(ApiConstants.S3_SOCKET_TIMEOUT) == null ? null
: Integer.valueOf(details.get(ApiConstants.S3_SOCKET_TIMEOUT)), imgStore.getCreated(), _configDao.getValue(Config.S3EnableRRS.toString()) == null ? false
- : Boolean.parseBoolean(_configDao.getValue(Config.S3EnableRRS.toString())), getMaxSingleUploadSizeInBytes());
+ : Boolean.parseBoolean(_configDao.getValue(Config.S3EnableRRS.toString())), getMaxSingleUploadSizeInBytes(),
+ details.get(ApiConstants.S3_CONNECTION_TTL) == null ? null : Integer.valueOf(details.get(ApiConstants.S3_CONNECTION_TTL)),
+ details.get(ApiConstants.S3_USE_TCP_KEEPALIVE) == null ? null : Boolean.parseBoolean(details.get(ApiConstants.S3_USE_TCP_KEEPALIVE)));
--- End diff --
Oh, I agree with you. It's just that I didn't want to touch the formatting for this patch.
When I looked at the code I had the same idea.
If this PR gets merged I want to send a second commit which fixes this.
---
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-8581: S3, make connection TTL ...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/582#issuecomment-121226423
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.
---