JENKINS-56740: Add multipart upload support for big files#88
JENKINS-56740: Add multipart upload support for big files#88bozaro wants to merge 2 commits intojenkinsci:masterfrom
Conversation
jglick
left a comment
jglick
left a comment
There was a problem hiding this comment.
Have some test failures and FindBugs errors. Sounds like this is a proof of concept but needs more work to become mergeable.
|
|
||
| /** A constant to define whether we should delete artifacts or leave them to be managed on the blob service side. */ | ||
| /** | ||
| * A constant to define whether we should delete artifacts or leave them to be managed on the blob service side. |
There was a problem hiding this comment.
Please minimize diffs when filing a PR—do not edit unrelated lines.
| public abstract URL toExternalURL(@NonNull Blob blob, @NonNull HttpMethod httpMethod) throws IOException; | ||
|
|
||
| @NonNull | ||
| public abstract MultipartUploader initiateMultipartUpload(@NonNull Blob blob) throws IOException; |
There was a problem hiding this comment.
Should not introduce new abstract methods.
There was a problem hiding this comment.
Unfortunately, I have not thought of anything better yet.
Uploading artifacts logic clearly does not fit into the existing interface. For multipart upload we need:
- check file size (worker), because we shoud known parts count and uploading time (multipart allowed only for files >= 5Mb);
- generate urls for everty part (master);
- initiate uploading to S3 (master);
- upload every part to S3 and store etag (worker);
- complete uploading to S3 (master).
Also we would like to make reduce "master <-> worker" roundtrips as possible.
There was a problem hiding this comment.
It would be more beautiful if I could request the URL to upload from worker, but:
- I don't know how to do it;
- it looks like as security hole.
| Map<String, URL> artifactUrls = new HashMap<>(); | ||
| LOGGER.log(Level.FINE, "Archiving from {0}: {1}", new Object[]{workspace, artifacts}); | ||
| BlobStore blobStore = getContext().getBlobStore(); | ||
| if (artifacts.isEmpty()) { |
There was a problem hiding this comment.
Under what conditions would this clause be hit? Normally the caller should skip and warn.
| try { | ||
| uploader.close(); | ||
| } catch (Exception e) { | ||
| listener.getLogger().printf("Can't abort multipart upload: %s", e.getMessage()); |
There was a problem hiding this comment.
Usually would use Functions.printStackTrace or similar.
There was a problem hiding this comment.
I don't know, how to use Functions.printStackTrace.
This error is not fatal and usually can be silently ignored. As I known, uncompleted multipart uploads should be removed by S3 bucket lifecycle policy.
| import com.amazonaws.auth.AWSStaticCredentialsProvider; | ||
| import com.amazonaws.services.s3.AmazonS3; | ||
| import com.amazonaws.services.s3.AmazonS3ClientBuilder; | ||
| import com.amazonaws.services.s3.model.*; |
There was a problem hiding this comment.
Try to avoid rearranging imports, and avoid wildcards. (Yes we should probably set up an import style enforcer or something.)
bozaro
force-pushed
the
upload-big-files
branch
from
77ebf73 to
9438ea6
Compare
|
Most of the errors are caused by the lack of implementation in the mock class. I got the impression that, from the Jenkins point of view, this whole structure is extremely unnatural. Because of this, it turns out some freak. |
|
Would it be possible to revive this PR? This is important to me |
3 participants
Proof-of-concept implementation.
I tried to implement the upload of large files. The code turned out so-so, but it works.
JENKINS-56740