I have done a code review, but not actually used this yet. I'm worried it's being looked at a bit late for the 2.7 branch, but I can't fault anyone but myself and other reviewers for that.
If we get it in, I'd like the markdown docs to highlight "experimental, unstable, use at own risk" for this option. Then 2.7 can be viewed as stabilisation of it, with goal being trusted by 2.8 & that warning removed. Perhaps a special subsection of s3a, "experiment fast output stream" and some specifics, rather than just listing a config flag, "fast", which everyone is going to turn on based on its name alone.
- check for and reject negative buffer size. Maybe have a check & Warn/reject too small a buffer?
- S3AFastOutputStream constructor should log @debug, not info
- would benefit from some comments explaining the two branches, e.g. "copy into buffer" and "buffer full, write it" + "recursively write remainder"
- If I created a 1-byte buffer, would that tail recursion trigger a stack overflow? If so, some checks in ctor may be good, such as just hard coded minimum & upping low values to it.
I'd swap the operations in the finally clause, so no matter what the superclass does, this close operation always finishes. Better yet, set close() earlier.
InterruptedException shouldn't be swallowed here, it hides problems and can cause process shutdowns to hang. Either convert to InterruptedIOException, or set interrupted flag on the current thread again.
Need a policy on interrupts here too. Imagine that communications with a remote S3 server are blocked, something has just sent a kill -3 to the client app, and is now waiting for it to finish cleanly...
can me made private
- Can we have flush() do a partial write? Would that gain anything in durability terms? At the very least, it may make timing of writes more consistent with other filesystems.
- I can think of some more tests here. If S3AFastOutputStream added a counter of #of uploads, a test could grab that stream and verify that writes triggered it, flushes triggered it, 0-byte-writes didn't, close cleaned up, etc. Also be interesting to time & compare classic vs fast operations & print that in the test results.