bamaer commented on PR #9158: URL: https://github.com/apache/hop/pull/9158#issuecomment-6516496227
Thanks @hansva. Built the aws plugin from this branch and ran `S3CommonPipedOutputStreamTest` (3/3 pass; the new write test fails without the fix). Code reviewed, not tested against a real bucket. **Should fix** - The message still doesn't contain the S3 error. With the log store initialised, as in a normal Hop run, a 403 from `createMultipartUpload` comes out as `s3 multipart exception caught (b/k)`, caused by `Pipe closed`. The task catches the exception and returns `false` (https://github.com/apache/hop/blob/6672b17709107de76ea2552c57a45c5045b7cb93/plugins/tech/aws/src/main/java/org/apache/hop/vfs/s3/s3common/S3CommonPipedOutputStream.java#L274-L292), so the cause never gets back to the writer. Suggested fix: after the abort, `throw e;` instead of `returnVal = false;`. The existing `ExecutionException` handling in `write()` and `close()` then gives `S3 multipart upload failed for b/k`, caused by `S3Exception: Access Denied ...`. I tried this locally and the three tests still pass. The tests currently pass because `LogChannel.GENERAL.logError` throws in the test JVM (log store not initialised), which sends them down the `ExecutionException` path. Adding `HopLogStore.init()` and asserting on the S3 message would cover the path users actually hit. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
