Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1424,6 +1424,13 @@ private Constants() {
*/
public static final String FS_S3A_CREATE_PERFORMANCE = "fs.s3a.create.performance";

/**
* Flag for commit if none match.
* This can be set in the {code createFile()} builder.
* Value {@value}.
*/
public static final String FS_S3A_CONDITIONAL_FILE_CREATE = "fs.s3a.conditional.file.create";

/**
* Default value for create performance in an S3A FS.
* Value {@value}.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
import static org.apache.hadoop.fs.s3a.Statistic.*;
import static org.apache.hadoop.fs.s3a.impl.HeaderProcessing.CONTENT_TYPE_OCTET_STREAM;
import static org.apache.hadoop.fs.s3a.impl.ProgressListenerEvent.*;
import static org.apache.hadoop.fs.s3a.impl.AWSHeaders.IF_NONE_MATCH;
import static org.apache.hadoop.fs.s3a.statistics.impl.EmptyS3AStatisticsContext.EMPTY_BLOCK_OUTPUT_STREAM_STATISTICS;
import static org.apache.hadoop.fs.statistics.impl.IOStatisticsBinding.trackDuration;
import static org.apache.hadoop.fs.statistics.impl.IOStatisticsBinding.trackDurationOfInvocation;
Expand Down Expand Up @@ -690,13 +691,23 @@ private long putObject() throws IOException {
final S3ADataBlocks.DataBlock block = getActiveBlock();
final long size = block.dataSize();
final S3ADataBlocks.BlockUploadData uploadData = block.startUpload();
final PutObjectRequest putObjectRequest =
PutObjectRequest putObjectRequest =
writeOperationHelper.createPutObjectRequest(
key,
uploadData.getSize(),
builder.putOptions);
clearActiveBlock();

PutObjectRequest.Builder maybeModifiedPutIfAbsentRequest = putObjectRequest.toBuilder();
Map<String, String> optionHeaders = builder.putOptions.getHeaders();

if (optionHeaders != null && optionHeaders.containsKey(IF_NONE_MATCH)) {
maybeModifiedPutIfAbsentRequest.overrideConfiguration(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as mentioned below as well, think we should upgrade SDK and then use the new .ifNoneMatch().

Also I would recommend you move all of this logic into a new private method in this class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, I would move this logic to RequestFactoryImpl. But you will need to add in a flag to the method signature, "isConditonalPutEnabled", and only add the if-none-match header in when sConditonalPutEnabled is true.

Basically, you only want to add if-none-match when the PUT is coming from S3ABlockOutputStream.close() and not from anywhere else.

override -> override.putHeader(IF_NONE_MATCH, optionHeaders.get(IF_NONE_MATCH)));
}

final PutObjectRequest finalizedRequest = maybeModifiedPutIfAbsentRequest.build();

BlockUploadProgress progressCallback =
new BlockUploadProgress(block, progressListener, now());
statistics.blockUploadQueued(size);
Expand Down Expand Up @@ -1389,6 +1400,11 @@ public static final class BlockOutputStreamBuilder {
*/
private boolean isMultipartUploadEnabled;

/**
* Is conditional create enables.
*/
private boolean isConditionalEnabled;

private BlockOutputStreamBuilder() {
}

Expand Down Expand Up @@ -1550,5 +1566,11 @@ public BlockOutputStreamBuilder withMultipartEnabled(
isMultipartUploadEnabled = value;
return this;
}

public BlockOutputStreamBuilder withConditionalEnabled(
final boolean value){
isConditionalEnabled = value;
return this;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -320,8 +320,7 @@ private CompleteMultipartUploadResponse finalizeMultipartUpload(
retrying,
() -> {
final CompleteMultipartUploadRequest.Builder requestBuilder =
getRequestFactory().newCompleteMultipartUploadRequestBuilder(
destKey, uploadId, partETags);
getRequestFactory().newCompleteMultipartUploadRequestBuilder(destKey, uploadId, partETags, putOptions);
return writeOperationHelperCallbacks.completeMultipartUpload(requestBuilder.build());
});
writeOperationHelperCallbacks.finishedWrite(destKey, length,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,12 +168,14 @@ CreateMultipartUploadRequest.Builder newMultipartUploadRequestBuilder(
* @param destKey destination object key
* @param uploadId ID of initiated upload
* @param partETags ordered list of etags
* @param putOptions options for the request
* @return the request builder.
*/
CompleteMultipartUploadRequest.Builder newCompleteMultipartUploadRequestBuilder(
String destKey,
String uploadId,
List<CompletedPart> partETags);
List<CompletedPart> partETags,
PutObjectOptions putOptions);

/**
* Create a HEAD object request builder.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public interface AWSHeaders {
String DATE = "Date";
String ETAG = "ETag";
String LAST_MODIFIED = "Last-Modified";
String IF_NONE_MATCH = "If-None-Match";

/*
* Amazon HTTP Headers used by S3A.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,19 +63,19 @@ public class CreateFileBuilder extends
* Classic create file option set: overwriting.
*/
public static final CreateFileOptions OPTIONS_CREATE_FILE_OVERWRITE =
new CreateFileOptions(CREATE_OVERWRITE_FLAGS, true, false, null);
new CreateFileOptions(CREATE_OVERWRITE_FLAGS, true, false, false, null);

/**
* Classic create file option set: no overwrite.
*/
public static final CreateFileOptions OPTIONS_CREATE_FILE_NO_OVERWRITE =
new CreateFileOptions(CREATE_NO_OVERWRITE_FLAGS, true, false, null);
new CreateFileOptions(CREATE_NO_OVERWRITE_FLAGS, true, false, false, null);

/**
* Performance create options.
*/
public static final CreateFileOptions OPTIONS_CREATE_FILE_PERFORMANCE =
new CreateFileOptions(CREATE_OVERWRITE_FLAGS, true, true, null);
new CreateFileOptions(CREATE_OVERWRITE_FLAGS, true, true, false, null);

/**
* Callback interface.
Expand Down Expand Up @@ -144,10 +144,12 @@ public FSDataOutputStream build() throws IOException {

final boolean performance =
options.getBoolean(Constants.FS_S3A_CREATE_PERFORMANCE, false);
final boolean conditionalCreate =
options.getBoolean(Constants.FS_S3A_CONDITIONAL_FILE_CREATE, false);
return callbacks.createFileFromBuilder(
path,
getProgress(),
new CreateFileOptions(flags, isRecursive(), performance, headers));
new CreateFileOptions(flags, isRecursive(), performance, conditionalCreate, headers));

}

Expand Down Expand Up @@ -218,6 +220,11 @@ public static final class CreateFileOptions {
*/
private final boolean performance;

/**
* conditional flag.
*/
private final boolean conditionalCreate;

/**
* Headers; may be null.
*/
Expand All @@ -227,16 +234,19 @@ public static final class CreateFileOptions {
* @param flags creation flags
* @param recursive create parent dirs?
* @param performance performance flag
* @param conditionalCreate conditional flag
* @param headers nullable header map.
*/
public CreateFileOptions(
final EnumSet<CreateFlag> flags,
final boolean recursive,
final boolean performance,
final boolean conditionalCreate,
final Map<String, String> headers) {
this.flags = flags;
this.recursive = recursive;
this.performance = performance;
this.conditionalCreate = conditionalCreate;
this.headers = headers;
}

Expand All @@ -246,6 +256,7 @@ public String toString() {
"flags=" + flags +
", recursive=" + recursive +
", performance=" + performance +
", conditionalCreate=" + conditionalCreate +
", headers=" + headers +
'}';
}
Expand All @@ -262,6 +273,10 @@ public boolean isPerformance() {
return performance;
}

public boolean isConditionalCreate() {
return conditionalCreate;
}

public Map<String, String> getHeaders() {
return headers;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import static org.apache.hadoop.fs.s3a.Constants.DIRECTORY_OPERATIONS_PURGE_UPLOADS;
import static org.apache.hadoop.fs.s3a.Constants.ENABLE_MULTI_DELETE;
import static org.apache.hadoop.fs.s3a.Constants.FIPS_ENDPOINT;
import static org.apache.hadoop.fs.s3a.Constants.FS_S3A_CONDITIONAL_FILE_CREATE;
import static org.apache.hadoop.fs.s3a.Constants.FS_S3A_CREATE_PERFORMANCE;
import static org.apache.hadoop.fs.s3a.Constants.FS_S3A_CREATE_PERFORMANCE_ENABLED;
import static org.apache.hadoop.fs.s3a.Constants.STORE_CAPABILITY_AWS_V2;
Expand Down Expand Up @@ -260,7 +261,7 @@ private InternalConstants() {
*/
public static final Set<String> CREATE_FILE_KEYS =
Collections.unmodifiableSet(
new HashSet<>(Arrays.asList(FS_S3A_CREATE_PERFORMANCE)));
new HashSet<>(Arrays.asList(FS_S3A_CREATE_PERFORMANCE, FS_S3A_CONDITIONAL_FILE_CREATE)));

/**
* Dynamic Path capabilities to be evaluated
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
import org.apache.hadoop.fs.s3a.auth.delegation.EncryptionSecrets;

import static org.apache.commons.lang3.StringUtils.isNotEmpty;
import static org.apache.hadoop.fs.s3a.impl.AWSHeaders.IF_NONE_MATCH;
import static org.apache.hadoop.fs.s3a.S3AEncryptionMethods.UNKNOWN_ALGORITHM;
import static org.apache.hadoop.fs.s3a.impl.InternalConstants.DEFAULT_UPLOAD_PART_COUNT_LIMIT;
import static org.apache.hadoop.util.Preconditions.checkArgument;
Expand Down Expand Up @@ -517,12 +518,20 @@ public CreateMultipartUploadRequest.Builder newMultipartUploadRequestBuilder(
public CompleteMultipartUploadRequest.Builder newCompleteMultipartUploadRequestBuilder(
String destKey,
String uploadId,
List<CompletedPart> partETags) {
List<CompletedPart> partETags,
PutObjectOptions putOptions) {

// a copy of the list is required, so that the AWS SDK doesn't
// attempt to sort an unmodifiable list.
CompleteMultipartUploadRequest.Builder requestBuilder =
CompleteMultipartUploadRequest.builder().bucket(bucket).key(destKey).uploadId(uploadId)
CompleteMultipartUploadRequest.Builder requestBuilder;
Map<String, String> optionHeaders = putOptions.getHeaders();

requestBuilder = CompleteMultipartUploadRequest.builder().bucket(bucket).key(destKey).uploadId(uploadId)
.multipartUpload(CompletedMultipartUpload.builder().parts(partETags).build());
if (optionHeaders != null && optionHeaders.containsKey(IF_NONE_MATCH)) {
requestBuilder = CompleteMultipartUploadRequest.builder().overrideConfiguration(
override ->override.putHeader(IF_NONE_MATCH, optionHeaders.get(IF_NONE_MATCH)));
}

return prepareRequest(requestBuilder);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.hadoop.fs.s3a.impl;

import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.fs.FSDataOutputStream;
import org.apache.hadoop.fs.FSDataOutputStreamBuilder;
import org.apache.hadoop.fs.FileSystem;
import org.apache.hadoop.fs.Path;
import org.apache.hadoop.fs.s3a.performance.AbstractS3ACostTest;
import org.apache.hadoop.fs.s3a.RemoteFileChangedException;
import org.apache.hadoop.fs.s3a.S3ATestUtils;
import org.apache.hadoop.io.IOUtils;

import org.assertj.core.api.Assertions;
import org.junit.Assert;
import org.junit.Test;
import software.amazon.awssdk.services.s3.model.S3Exception;

import java.io.IOException;
import static org.apache.hadoop.fs.contract.ContractTestUtils.dataset;
import static org.apache.hadoop.fs.s3a.Constants.FAST_UPLOAD_BUFFER;
import static org.apache.hadoop.fs.s3a.Constants.FAST_UPLOAD_BUFFER_ARRAY;
import static org.apache.hadoop.fs.s3a.Constants.FS_S3A_CONDITIONAL_FILE_CREATE;
import static org.apache.hadoop.fs.s3a.Constants.MIN_MULTIPART_THRESHOLD;
import static org.apache.hadoop.fs.s3a.Constants.MULTIPART_MIN_SIZE;
import static org.apache.hadoop.fs.s3a.Constants.MULTIPART_SIZE;
import static org.apache.hadoop.fs.s3a.S3ATestUtils.skipIfNotEnabled;
import static org.apache.hadoop.fs.s3a.S3ATestUtils.removeBaseAndBucketOverrides;
import static org.apache.hadoop.fs.s3a.impl.InternalConstants.UPLOAD_PART_COUNT_LIMIT;
import static org.apache.hadoop.fs.s3a.scale.ITestS3AMultipartUploadSizeLimits.MPU_SIZE;
import static org.apache.hadoop.fs.s3a.scale.S3AScaleTestBase._1MB;


public class ITestS3APutIfMatch extends AbstractS3ACostTest {

private Configuration conf;

@Override
public Configuration createConfiguration() {
Configuration conf = super.createConfiguration();
S3ATestUtils.disableFilesystemCaching(conf);
removeBaseAndBucketOverrides(conf,
MULTIPART_SIZE,
UPLOAD_PART_COUNT_LIMIT);
conf.setLong(MULTIPART_SIZE, MPU_SIZE);
conf.setLong(UPLOAD_PART_COUNT_LIMIT, 2);
conf.setLong(MIN_MULTIPART_THRESHOLD, MULTIPART_MIN_SIZE);
conf.setInt(MULTIPART_SIZE, MULTIPART_MIN_SIZE);
conf.set(FAST_UPLOAD_BUFFER, getBlockOutputBufferName());
return conf;
}

@Override
public void setup() throws Exception {
super.setup();
conf = createConfiguration();
skipIfNotEnabled(conf, FS_S3A_CONDITIONAL_FILE_CREATE,
"Skipping IfNoneMatch tests");
}

protected String getBlockOutputBufferName() {
return FAST_UPLOAD_BUFFER_ARRAY;
}

/**
* Create a file using the PutIfMatch feature from S3
* @param fs filesystem
* @param path path to write
* @param data source dataset. Can be null
* @throws IOException on any problem
*/
private static void createFileWithIfNoneMatchFlag(FileSystem fs,
Path path,
byte[] data,
String ifMatchTag) throws Exception {
FSDataOutputStreamBuilder builder = fs.createFile(path);
builder.must(FS_S3A_CONDITIONAL_FILE_CREATE, ifMatchTag);
FSDataOutputStream stream = builder.create().build();
if (data != null && data.length > 0) {
stream.write(data);
}
stream.close();
IOUtils.closeStream(stream);
}

@Test
public void testPutIfAbsentConflict() throws IOException {
FileSystem fs = getFileSystem();
Path testFile = methodPath();

fs.mkdirs(testFile.getParent());
byte[] fileBytes = dataset(TEST_FILE_LEN, 0, 255);

try {
createFileWithIfNoneMatchFlag(fs, testFile, fileBytes, "*");
createFileWithIfNoneMatchFlag(fs, testFile, fileBytes, "*");
} catch (Exception e) {
Assert.assertEquals(RemoteFileChangedException.class, e.getClass());

S3Exception s3Exception = (S3Exception) e.getCause();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was gong to point you at GenericTestUtils.assertExceptionContains() but it doesn't validate the cause type or return the value as a cast type the way intercept() does. Time for a new assert there.

Assertions.assertThat(s3Exception.statusCode()).isEqualTo(412);
}
}


@Test
public void testPutIfAbsentLargeFileConflict() throws IOException {
FileSystem fs = getFileSystem();
Path testFile = methodPath();

// enough bytes for Multipart Upload
byte[] fileBytes = dataset(6 * _1MB, 'a', 'z' - 'a');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are ways to do this with small files, this matters because a 6MB file is large enough that it'd have to become a scale test. Yes, it is a scale test when people are testing across the planet.

Do not worry about this right now, but be aware before merging into trunk the large file test will have to be moved to a subset of S3AScaleTestBase, and this test replaced with something using a write to a magic path –or even better, we add another new create file option to force multipart uploads always.

Designing for that makes me think that a followup to this should move to an enumset of CreateFile flags


try {
createFileWithIfNoneMatchFlag(fs, testFile, fileBytes, "*");
createFileWithIfNoneMatchFlag(fs, testFile, fileBytes, "*");
} catch (Exception e) {
Assert.assertEquals(RemoteFileChangedException.class, e.getClass());

// Error gets caught here:
S3Exception s3Exception = (S3Exception) e.getCause();
Assertions.assertThat(s3Exception.statusCode()).isEqualTo(412);
}
}
}
Loading