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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
