refactor: adds specific exception handling for the download operation

[yurimssilva]

What this PR changes/adds

• Enhanced exception handling and incorporated explicit error messages for both download and upload operations, during the transfer process.

Why it does that

• The previous implementation lacked clarity in distinguishing errors between fetching and pushing operations, potentially leading to confusion during troubleshooting.

Linked Issue(s)

Closes #200

[ndr-brt]

I'd suggest to refactor the data sink a little, fetch and upload are too coupled, my suggestion would be to reach something like this:

for (var part : parts) {
    try (var input = part.openStream()) {
        // fetch logic
    } (catch Exception e) {
        return fetchFailure();
    }

    try {
        // upload logic
    } catch (Exception e) {
        return uploadFailure(e);
    }
}

that will clearly distinguish between different kind of exception without catching and retrowing exceptions.

An additional question would be: is it correct that a multi-parts upload should be stopped at the first failure? maybe yes, but that should be eventually documented with a test that explains why

[yurimssilva]

I'd suggest to refactor the data sink a little, fetch and upload are too coupled, my suggestion would be to reach something like this:

for (var part : parts) {
    try (var input = part.openStream()) {
        // fetch logic
    } (catch Exception e) {
        return fetchFailure();
    }

    try {
        // upload logic
    } catch (Exception e) {
        return uploadFailure(e);
    }
}

that will clearly distinguish between different kind of exception without catching and retrowing exceptions.

An additional question would be: is it correct that a multi-parts upload should be stopped at the first failure? maybe yes, but that should be eventually documented with a test that explains why

The coupling between DataSource and DataSink complicates the neat separation of download and upload logic. The input resource, required outside the download scope, must be closed in any situation. I addressed this by just isolating the upload logic to handle errors within that block differently.

Regarding stopping multi-parts upload at the first failure, I think considering a potential retry strategy might be beneficial. Should we open a new issue to discuss and address this?

[ndr-brt]

I'd suggest to refactor the data sink a little, fetch and upload are too coupled, my suggestion would be to reach something like this:

for (var part : parts) {
    try (var input = part.openStream()) {
        // fetch logic
    } (catch Exception e) {
        return fetchFailure();
    }

    try {
        // upload logic
    } catch (Exception e) {
        return uploadFailure(e);
    }
}

that will clearly distinguish between different kind of exception without catching and retrowing exceptions.
An additional question would be: is it correct that a multi-parts upload should be stopped at the first failure? maybe yes, but that should be eventually documented with a test that explains why

The coupling between DataSource and DataSink complicates the neat separation of download and upload logic. The input resource, required outside the download scope, must be closed in any situation. I addressed this by just isolating the upload logic to handle errors within that block differently.

the input can be transformed in a list of UploadPartRequest or RequestBody, then it can be closed before any of the upload call is made (createMultipartUpload, uploadPart, completeMultipartUpload), so the two parts can be logically split.

maybe that's not really efficient, but, in alternative, a custom exception SourceException or so could be defined and be thrown by the openStream method, so it can caught separately from all the others in the transferParts method, that will avoid the nested try catch block.

Please note that, an exception thrown by the input.readNBytes will be considered as uploadFailure (also in your code), to avoid that, the S3Part could return a InputStream decorator that catch and throws the SourceException I mentioned also for the readNBytes method.

Regarding stopping multi-parts upload at the first failure, I think considering a potential retry strategy might be beneficial. Should we open a new issue to discuss and address this?

yes, that makes sense

[ndr-brt]

some cleanup needed, good otherwise

[ndr-brt]

just a missing test, please fix dependencies and then we can merge this

[ndr-brt]

LGTM, could you please fix the dependency check?