cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From wilderrodrigues <...@git.apache.org>
Subject [GitHub] cloudstack pull request: CLOUDSTACK-9062: Improve S3 implementatio...
Date Thu, 19 Nov 2015 12:46:28 GMT
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.
---

Mime
View raw message