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/11/18 01:08:28 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-9062: Improve S3 implementatio...
GitHub user borisroman opened a pull request:
https://github.com/apache/cloudstack/pull/1083
CLOUDSTACK-9062: Improve S3 implementation.
The S3 implementation is far from finished, this commit focuses on the bases.
- Upgrade AWS SDK to latest version.
- Rewrite S3 Template downloader.
- Rewrite S3Utils utility class.
- Improve addImageStoreS3 API command.
- Split various classes for convenience.
- Various minor improvements and code optimizations.
A side effect of the new AWS SDK is that it, by default, uses the V4 signature. Therefore I added an option to specify the Signer, so it stays compatible with previous versions.
Please review thoroughly, both code inspection and (automated) integration tests. Currently no integration tests are available specifically for S3. Therefore the implementation is needed to be tested manually, for now...
What I tested:
- Greenfield install -> will download latest systemvm template automatically to S3.
- Upload a template/iso
- Download a template/iso
- Restart of management server -> list available templates -> doesn't download them again if available.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/borisroman/cloudstack CLOUDSTACK-9062
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1083.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 #1083
----
commit 8ca0d0730a5739a209dbf7f138ed5e5c482783d8
Author: Boris Schrijver <bo...@pcextreme.nl>
Date: 2015-11-13T01:19:24Z
CLOUDSTACK-9062: Improve S3 implementation.
The S3 implementation is far from finished, this commit focusses on the bases.
- Upgrade AWS SDK to latest version.
- Rewrite S3 Template downloader.
- Rewrite S3Utils utility class.
- Improve addImageStoreS3 API command.
- Split various classes for convenience.
- Various minor improvements and code optimalisations.
A side effect of the new AWS SDK is that it, by default, uses the V4 signature. Therefore I added an option to specify the Signer, so it stays compatible with previous versions.
----
---
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-9062: Improve S3 implementatio...
Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on the pull request:
https://github.com/apache/cloudstack/pull/1083#issuecomment-158062783
@wilderrodrigues Fixed your comments, refactoring is for the next round ;)
---
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-9062: Improve S3 implementatio...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1083#issuecomment-158048085
@borisroman @wido I made some comments in the PR. Those are not blocking, but would be very fruitful to get them in place in a second round of the refactor.
The public constructor and hashCode()/equals() methods can be address before the merge. But the refactor can take place within a couple of weeks.
@borisroman: feel free to drop by on 1st December, or any other date on that week, so we can do it together.
It LGTM :+1: based on my code review.
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-9062: Improve S3 implementatio...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1083#issuecomment-157683314
@borisroman checkstyle reports unused imports
---
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-9062: Improve S3 implementatio...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1083
---
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-9062: Improve S3 implementatio...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1083#issuecomment-158328981
LGTM. Reviewed the code and looks good.
---
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-9062: Improve S3 implementatio...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1083#discussion_r45335015
--- Diff: api/src/com/cloud/agent/api/to/S3TO.java ---
@@ -155,6 +161,7 @@ public int hashCode() {
result = 31 * result + (secretKey != null ? secretKey.hashCode() : 0);
--- End diff --
hasCode() and toString methods should be at the end of the file. The last methods.
---
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-9062: Improve S3 implementatio...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1083#issuecomment-157657472
Initially it looks good, but with S3 it's very hard to test this. At a first glance the code looks good. Much better then it was
---
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-9062: Improve S3 implementatio...
Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on the pull request:
https://github.com/apache/cloudstack/pull/1083#issuecomment-158344376
@remibergsma Of course I tested manually! Nice Jenkins Job btw ;)
I see three LGTM's, two of which are based on code review, one on running integration tests (which showed nothing broke). I tested the changes in a physical setup, with S3 storage facilities. It does the same as before, so the refactor went well.
Do we merge, or wait for someone else to test? @wido @remibergsma @wilderrodrigues
---
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-9062: Improve S3 implementatio...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1083#discussion_r45335326
--- Diff: api/src/org/apache/cloudstack/api/command/admin/storage/AddImageStoreS3CMD.java ---
@@ -201,15 +209,15 @@ public int hashCode() {
result = 31 * result + (secretKey != null ? secretKey.hashCode() : 0);
result = 31 * result + (endPoint != null ? endPoint.hashCode() : 0);
result = 31 * result + (bucketName != null ? bucketName.hashCode() : 0);
- result = 31 * result + (httpsFlag != null && httpsFlag == true ? 1 : 0);
+ result = 31 * result + (signer != null ? signer.hashCode() : 0);
--- End diff --
The same here: would be nice to move hashCode() and equals() methods to the end of the file.
---
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-9062: Improve S3 implementatio...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1083#discussion_r45334945
--- Diff: api/src/com/cloud/agent/api/to/S3TO.java ---
@@ -51,8 +52,8 @@ public S3TO() {
}
public S3TO(final Long id, final String uuid, final String accessKey, final String secretKey, final String endPoint, final String bucketName,
- final Boolean httpsFlag, final Integer connectionTimeout, final Integer maxErrorRetry, final Integer socketTimeout, final Date created,
- final boolean enableRRS, final long maxUploadSize, final Integer connectionTtl, final Boolean useTCPKeepAlive) {
+ final String signer, final Boolean httpsFlag, final Integer connectionTimeout, final Integer maxErrorRetry, final Integer socketTimeout,
+ final Date created, final boolean enableRRS, final long maxUploadSize, final Integer connectionTtl, final Boolean useTCPKeepAlive) {
super();
--- End diff --
You can remove this call and also remove the public default constructor: it's not used.
---
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-9062: Improve S3 implementatio...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1083#issuecomment-158341279
LGTM, based on a set of tests that I run on this branch (which I rebased myself first). Screenshot is from an experimental Jenkins job (that runs the same tests I did manually until now).
These test do not cover your change, all they do is show you didn't break them. Someone else needs to review the code and we should do some (manual?) S3 tests. Or did you do that already @borisroman ?
---
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-9062: Improve S3 implementatio...
Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on the pull request:
https://github.com/apache/cloudstack/pull/1083#issuecomment-157551229
Ping @wido @remibergsma @wilderrodrigues @karuturi
---
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-9062: Improve S3 implementatio...
Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on the pull request:
https://github.com/apache/cloudstack/pull/1083#issuecomment-157697285
@DaanHoogland You're right. Forgot to remove it in my last iteration. Thanks!
---
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-9062: Improve S3 implementatio...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1083#discussion_r45335913
--- Diff: core/src/com/cloud/storage/template/S3TemplateDownloader.java ---
@@ -342,47 +274,38 @@ public InputStream getS3ObjectInputStream() {
return null;
}
- return S3Utils.getObjectStream(s3to, s3to.getBucketName(), s3Key);
+ return S3Utils.getObjectStream(s3TO, s3TO.getBucketName(), s3Key);
}
public void cleanupAfterError() {
- if (status != Status.UNRECOVERABLE_ERROR) {
- s_logger.debug("S3Template downloader does not have state UNRECOVERABLE_ERROR, no cleanup neccesarry.");
- return;
- }
-
- s_logger.info("Cleanup after UNRECOVERABLE_ERROR, trying to remove object: " + s3Key);
+ LOGGER.warn("Cleanup after error, trying to remove object: " + s3Key);
- S3Utils.deleteObject(s3to, s3to.getBucketName(), s3Key);
+ S3Utils.deleteObject(s3TO, s3TO.getBucketName(), s3Key);
}
@Override
- @SuppressWarnings("fallthrough")
public boolean stopDownload() {
- switch (getStatus()) {
- case IN_PROGRESS:
- if (request != null) {
- request.abort();
- }
- status = TemplateDownloader.Status.ABORTED;
- return true;
- case UNKNOWN:
- case NOT_STARTED:
- case RECOVERABLE_ERROR:
- case UNRECOVERABLE_ERROR:
- case ABORTED:
- status = TemplateDownloader.Status.ABORTED;
- case DOWNLOAD_FINISHED:
- try {
- S3Utils.deleteObject(s3to, s3to.getBucketName(), s3Key);
- } catch (Exception ex) {
- // ignore delete exception if it is not there
- }
- return true;
-
- default:
- return true;
+ switch (status) {
--- End diff --
Same here: let's refactor this in the a second round. Now we can merge, but let's 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-9062: Improve S3 implementatio...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1083#discussion_r45187932
--- Diff: core/src/com/cloud/storage/template/S3TemplateDownloader.java ---
@@ -19,303 +19,236 @@
package com.cloud.storage.template;
-import static com.cloud.utils.StringUtils.join;
-import static java.util.Arrays.asList;
-
-import java.io.BufferedInputStream;
-import java.io.IOException;
-import java.io.InputStream;
-import java.util.Date;
-
+import com.amazonaws.event.ProgressEvent;
+import com.amazonaws.event.ProgressEventType;
+import com.amazonaws.event.ProgressListener;
+import com.amazonaws.services.s3.model.ObjectMetadata;
+import com.amazonaws.services.s3.model.PutObjectRequest;
+import com.amazonaws.services.s3.model.StorageClass;
+import com.amazonaws.services.s3.transfer.Upload;
+import com.cloud.agent.api.to.S3TO;
+import com.cloud.utils.net.HTTPUtils;
+import com.cloud.utils.net.Proxy;
+import com.cloud.utils.storage.S3.S3Utils;
import org.apache.cloudstack.managed.context.ManagedContextRunnable;
import org.apache.cloudstack.storage.command.DownloadCommand.ResourceType;
import org.apache.commons.httpclient.ChunkedInputStream;
--- End diff --
no longer used?
---
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-9062: Improve S3 implementatio...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1083#discussion_r45335845
--- Diff: core/src/com/cloud/storage/template/S3TemplateDownloader.java ---
@@ -19,303 +19,235 @@
package com.cloud.storage.template;
-import static com.cloud.utils.StringUtils.join;
-import static java.util.Arrays.asList;
-
-import java.io.BufferedInputStream;
-import java.io.IOException;
-import java.io.InputStream;
-import java.util.Date;
-
+import com.amazonaws.event.ProgressEvent;
+import com.amazonaws.event.ProgressEventType;
+import com.amazonaws.event.ProgressListener;
+import com.amazonaws.services.s3.model.ObjectMetadata;
+import com.amazonaws.services.s3.model.PutObjectRequest;
+import com.amazonaws.services.s3.model.StorageClass;
+import com.amazonaws.services.s3.transfer.Upload;
+import com.cloud.agent.api.to.S3TO;
+import com.cloud.utils.net.HTTPUtils;
+import com.cloud.utils.net.Proxy;
+import com.cloud.utils.storage.S3.S3Utils;
import org.apache.cloudstack.managed.context.ManagedContextRunnable;
import org.apache.cloudstack.storage.command.DownloadCommand.ResourceType;
-import org.apache.commons.httpclient.ChunkedInputStream;
-import org.apache.commons.httpclient.Credentials;
import org.apache.commons.httpclient.Header;
import org.apache.commons.httpclient.HttpClient;
-import org.apache.commons.httpclient.HttpException;
-import org.apache.commons.httpclient.HttpMethod;
-import org.apache.commons.httpclient.HttpMethodRetryHandler;
-import org.apache.commons.httpclient.HttpStatus;
-import org.apache.commons.httpclient.MultiThreadedHttpConnectionManager;
-import org.apache.commons.httpclient.NoHttpResponseException;
-import org.apache.commons.httpclient.UsernamePasswordCredentials;
-import org.apache.commons.httpclient.auth.AuthScope;
+import org.apache.commons.httpclient.URIException;
import org.apache.commons.httpclient.methods.GetMethod;
import org.apache.commons.httpclient.params.HttpMethodParams;
import org.apache.commons.lang.StringUtils;
import org.apache.log4j.Logger;
-import com.amazonaws.AmazonClientException;
-import com.amazonaws.services.s3.model.ObjectMetadata;
-import com.amazonaws.services.s3.model.ProgressEvent;
-import com.amazonaws.services.s3.model.ProgressListener;
-import com.amazonaws.services.s3.model.PutObjectRequest;
-import com.amazonaws.services.s3.model.StorageClass;
-import com.cloud.agent.api.storage.Proxy;
-import com.cloud.agent.api.to.S3TO;
-import com.cloud.utils.Pair;
-import com.cloud.utils.S3Utils;
-import com.cloud.utils.UriUtils;
+import java.io.BufferedInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.Date;
+
+import static com.cloud.utils.StringUtils.join;
+import static java.util.Arrays.asList;
/**
* Download a template file using HTTP(S)
+ *
+ * This class, once instantiated, has the purpose to download a single Template to an S3 Image Store.
+ *
+ * Execution of the instance is started when runInContext() is called.
*/
public class S3TemplateDownloader extends ManagedContextRunnable implements TemplateDownloader {
- private static final Logger s_logger = Logger.getLogger(S3TemplateDownloader.class.getName());
- private static final MultiThreadedHttpConnectionManager s_httpClientManager = new MultiThreadedHttpConnectionManager();
-
- private String downloadUrl;
- private String installPath;
- private String s3Key;
- private String fileName;
- private String fileExtension;
- private String errorString = " ";
-
+ private static final Logger LOGGER = Logger.getLogger(S3TemplateDownloader.class.getName());
+
+ private final String downloadUrl;
+ private final String s3Key;
+ private final String fileExtension;
+ private final HttpClient httpClient;
+ private final GetMethod getMethod;
+ private final DownloadCompleteCallback downloadCompleteCallback;
+ private final S3TO s3TO;
+ private String errorString = "";
private TemplateDownloader.Status status = TemplateDownloader.Status.NOT_STARTED;
private ResourceType resourceType = ResourceType.TEMPLATE;
- private final HttpClient client;
- private final HttpMethodRetryHandler myretryhandler;
- private GetMethod request;
- private DownloadCompleteCallback completionCallback;
- private S3TO s3to;
-
- private long remoteSize = 0;
- private long downloadTime = 0;
+ private long remoteSize;
+ private long downloadTime;
private long totalBytes;
private long maxTemplateSizeInByte;
private boolean resume = false;
- private boolean inited = true;
- public S3TemplateDownloader(S3TO s3to, String downloadUrl, String installPath, DownloadCompleteCallback callback,
- long maxTemplateSizeInBytes, String user, String password, Proxy proxy, ResourceType resourceType) {
- this.s3to = s3to;
+ public S3TemplateDownloader(S3TO s3TO, String downloadUrl, String installPath, DownloadCompleteCallback downloadCompleteCallback,
+ long maxTemplateSizeInBytes, String username, String password, Proxy proxy, ResourceType resourceType) {
this.downloadUrl = downloadUrl;
- this.installPath = installPath;
- this.status = TemplateDownloader.Status.NOT_STARTED;
+ this.s3TO = s3TO;
this.resourceType = resourceType;
this.maxTemplateSizeInByte = maxTemplateSizeInBytes;
+ this.httpClient = HTTPUtils.getHTTPClient();
+ this.downloadCompleteCallback = downloadCompleteCallback;
- this.totalBytes = 0;
- this.client = new HttpClient(s_httpClientManager);
+ // Create a GET method for the download url.
+ this.getMethod = new GetMethod(downloadUrl);
- this.myretryhandler = new HttpMethodRetryHandler() {
- @Override
- public boolean retryMethod(final HttpMethod method, final IOException exception, int executionCount) {
- if (executionCount >= 2) {
- // Do not retry if over max retry count
- return false;
- }
- if (exception instanceof NoHttpResponseException) {
- // Retry if the server dropped connection on us
- return true;
- }
- if (!method.isRequestSent()) {
- // Retry if the request has not been sent fully or
- // if it's OK to retry methods that have been sent
- return true;
- }
- // otherwise do not retry
- return false;
- }
- };
+ // Set the retry handler, default to retry 5 times.
+ this.getMethod.getParams().setParameter(HttpMethodParams.RETRY_HANDLER, HTTPUtils.getHttpMethodRetryHandler(5));
- try {
- request = new GetMethod(downloadUrl);
- request.getParams().setParameter(HttpMethodParams.RETRY_HANDLER, myretryhandler);
- completionCallback = callback;
-
- Pair<String, Integer> hostAndPort = UriUtils.validateUrl(downloadUrl);
- fileName = StringUtils.substringAfterLast(downloadUrl, "/");
- fileExtension = StringUtils.substringAfterLast(fileName, ".");
-
- if (proxy != null) {
- client.getHostConfiguration().setProxy(proxy.getHost(), proxy.getPort());
- if (proxy.getUserName() != null) {
- Credentials proxyCreds = new UsernamePasswordCredentials(proxy.getUserName(), proxy.getPassword());
- client.getState().setProxyCredentials(AuthScope.ANY, proxyCreds);
- }
- }
- if ((user != null) && (password != null)) {
- client.getParams().setAuthenticationPreemptive(true);
- Credentials defaultcreds = new UsernamePasswordCredentials(user, password);
- client.getState().setCredentials(
- new AuthScope(hostAndPort.first(), hostAndPort.second(), AuthScope.ANY_REALM), defaultcreds);
- s_logger.info("Added username=" + user + ", password=" + password + "for host " + hostAndPort.first()
- + ":" + hostAndPort.second());
- } else {
- s_logger.info("No credentials configured for host=" + hostAndPort.first() + ":" + hostAndPort.second());
- }
- } catch (IllegalArgumentException iae) {
- errorString = iae.getMessage();
- status = TemplateDownloader.Status.UNRECOVERABLE_ERROR;
- inited = false;
- } catch (Exception ex) {
- errorString = "Unable to start download -- check url? ";
- status = TemplateDownloader.Status.UNRECOVERABLE_ERROR;
- s_logger.warn("Exception in constructor -- " + ex.toString());
- } catch (Throwable th) {
- s_logger.warn("throwable caught ", th);
- }
+ // Follow redirects
+ this.getMethod.setFollowRedirects(true);
+
+ // Set file extension.
+ this.fileExtension = StringUtils.substringAfterLast(StringUtils.substringAfterLast(downloadUrl, "/"), ".");
+
+ // Calculate and set S3 Key.
+ this.s3Key = join(asList(installPath, StringUtils.substringAfterLast(downloadUrl, "/")), S3Utils.SEPARATOR);
+
+ // Set proxy if available.
+ HTTPUtils.setProxy(proxy, this.httpClient);
+
+ // Set credentials if available.
+ HTTPUtils.setCredentials(username, password, this.httpClient);
}
@Override
public long download(boolean resume, DownloadCompleteCallback callback) {
- switch (status) {
- case ABORTED:
- case UNRECOVERABLE_ERROR:
- case DOWNLOAD_FINISHED:
+ if (!status.equals(Status.NOT_STARTED)) {
--- End diff --
If you want, after this PR is merged, please come to SBP and the two of us can have a Refactoring Hackathon and make this better. It's fine by now, but we could gain some more readability and extensibility if we decouple part of the code. The status, for example, should be in the Callbacl that is in the parameter list. It could also be used to delated to an adapter which action to take once the callback returns.
With only 2 small changes we could get rid of all those IFs and anonymous classes.
---
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-9062: Improve S3 implementatio...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1083#discussion_r45335103
--- Diff: api/src/org/apache/cloudstack/api/command/admin/storage/AddImageStoreS3CMD.java ---
@@ -119,17 +129,16 @@ public void execute() throws ResourceUnavailableException, InsufficientCapacityE
dm.put(ApiConstants.S3_USE_TCP_KEEPALIVE, getUseTCPKeepAlive().toString());
}
-
try{
ImageStore result = _storageService.discoverImageStore(null, null, "S3", null, dm);
- ImageStoreResponse storeResponse = null;
+ ImageStoreResponse storeResponse;
--- End diff --
Clever... loved 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.
---