Thanks Alex for your detailed feedback. much appreciated. I agree on most of them.
>are the node types required in the jcr/resource bundle, in a cnd which otherwise only defines the sling:Resource node type?
No. Yes I think it can be moved to slingpostbundle.
>IMO the :applyToChunks flag is not necessary, if you want to abort, you simple DELETE (or :operation=delete) the target file
It is required in update case where you don't want to delete original file. As on now, delete chunks on new file results in empty nt:file. It should delete the file itself. wdyt?
>Also, we cannot rely that this is always sent correctly by the client, the usual "abort" case will be that the upload has been aborted and nothing else is happening anymore. We need to handle partially uploaded nt:file nodes in some way then... garbage collection?
Yes I agree. Scheduled job is required which deletes all partial uploads which are 12hrs older. frequency of job say 6hrs. It would be configurable anyway.
>only sequential chunk upload allowed (chunkOffset != currentLength)
Yes. parallel not supported as of now. there would be other considerations too for parallel chunks in current implementation.
As per few first comments on this issue, community doesn't support idea of parallel chunk upload, although my experiment with cq deployed on AWS cloud clearly demonstrated the advantage . As per your comment too "Agree with Julian & Felix: #2 (resume broken upload) makes sense, but I don't get why parallel uploads should help. What was used as server/endpoint for the above comparison?"
I would be happy to support parallel chunk upload, if community agrees to it.
>the totalLength != expectedLength check in line 286 seems wrong to me
File length sent in first chunk upload request is persisted in "sling:fileLength". now on subsequent requests the @Length parameter is validated with the persisted value. if there is mismatch, proper exception is thrown. Total file length cannot change between first and subsequent chunk upload request.
>What else is the need for the sling:length property then? IMO it's not needed, too easy to get out of sync with what's in the sling:chunk child nodes anyway
yes it can be derived. you need to read all chunks and add their respecitive sizes. I kept it simple to keep it in a property. Also user can see current size from UI anytime.
>Is this thread-safe? Assuming multiple chunk requests coming in in parallel.
Yes unless they do on same node. Test cases SlingPostChunkUploadTest#testConcurrentChunkUpload and SlingPostChunkUploadTest#testChunkUploadOnExistingNode covered the scenario.
>Merging is done through a temporary file stored on the file system.
yes this can be improved.
> RequestProperty extensions (getOffset, getLength, etc.) might be useful to separate out into a Chunk object..
Yes it can be re-factored.