AWS: support S3 strong consistency

[jackye1995]

AWS released S3 strong consistency, and now all LIST and GET are strongly consistent. So we can use a LIST to force strong consistency before performing a HEAD for exists() check.

More details: https://aws.amazon.com/blogs/aws/amazon-s3-update-strong-read-after-write-consistency/

[rdblue]

It looks like it should be possible to use just a LIST call here because we don't need the full metadata from the HEAD request. Metadata is only used for the existence check and for the file size, which should be available from the list response:

// list object to force strong consistencyListObjectsV2Responseresponse = client().listObjectsV2(ListObjectsV2Request.builder()
.bucket(uri().bucket())
.prefix(uri().key())
.build());
Preconditions.checkState(response.contents().size() <= 1, "Invalid file: %s contains more than one object", uri);
this.exists = response.contents().size() > 0;
this.length = response.contents().get(0).size();

[danielcweeks]

I agree with @rdblue's comment that we probably don't want to do two requests and LIST alone would be sufficient.

Also, I'm not clear on how forcing a LIST would ensure correctness for a following head call. Based on the docs I didn't see anything about that sequencing resulting in HEAD consistency.

[jackye1995]

It looks like it should be possible to use just a LIST call here because we don't need the full metadata from the HEAD request. Metadata is only used for the existence check and for the file size, which should be available from the list response:

// list object to force strong consistencyListObjectsV2Responseresponse = client().listObjectsV2(ListObjectsV2Request.builder()
.bucket(uri().bucket())
.prefix(uri().key())
.build());
Preconditions.checkState(response.contents().size() <= 1, "Invalid file: %s contains more than one object", uri);
this.exists = response.contents().size() > 0;
this.length = response.contents().get(0).size();

Thanks for the suggestion, did not know LIST also gets the length metadata.

[danielcweeks]

I think this changes the exception behavior a little bit because now S3InputFile.getLength() will throw NPE as opposed to S3 NOT_FOUND in the event it does not exist and getLength() is called. Not a huge issue, but we might consider improving that a little.

[jackye1995]

Yeah I was going to ask, I saw in HadoopInputFile it simply throws a RuntimeIOException when asking for length of a file not exist. Probably better to check existence and throw NotFoundException in this case.

[rdblue]

I agree, NotFoundException would be much better.

[kbendick]

Would it make sense to test for the contents of a file change after put (and not just the length being the same like is currently tested)?

[jackye1995]

Would it make sense to test for the contents of a file change after put (and not just the length being the same like is currently tested)?

What situation does it try to cover in the context of this PR?
(I do have more tests for S3FileIO that I plan to port here in another PR, which checks the contents of a file as you describe)

[danielcweeks]

Thanks @jackye1995