Skip to content

Commit f4dcedf

Browse files
committed
HADOOP-18925. S3A: option "fs.s3a.optimized.copy.from.local.enabled"
* Address review comments, including renaming the option to to fs.s3a.optimized.copy.from.local.enabled * make state of the option a path capability, with test in ITestS3ACopyFromLocalFile * And disable filesystem caching in that suite so the new test passes, and the other tests run with the correct setting. Change-Id: I60965b489f81cdd7271bf6cd83a1d1b182cbe3ca
1 parent 46bf12b commit f4dcedf

File tree

4 files changed

+65
-35
lines changed

4 files changed

+65
-35
lines changed

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Constants.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1315,11 +1315,11 @@ private Constants() {
13151315
* This switch allows for it to be disabled if there are problems.
13161316
* Value: {@value}.
13171317
*/
1318-
public static final String COPY_FROM_LOCAL_ENABLED = "fs.s3a.copy.from.local.enabled";
1318+
public static final String OPTIMIZED_COPY_FROM_LOCAL = "fs.s3a.optimized.copy.from.local.enabled";
13191319

13201320
/**
1321-
* Default value for {@link #COPY_FROM_LOCAL_ENABLED}.
1321+
* Default value for {@link #OPTIMIZED_COPY_FROM_LOCAL}.
13221322
* Value: {@value}.
13231323
*/
1324-
public static final boolean COPY_FROM_LOCAL_ENABLED_DEFAULT = true;
1324+
public static final boolean OPTIMIZED_COPY_FROM_LOCAL_DEFAULT = true;
13251325
}

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java

Lines changed: 43 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -463,7 +463,7 @@ public class S3AFileSystem extends FileSystem implements StreamCapabilities,
463463
* Flag to indicate that the higher performance copyFromLocalFile implementation
464464
* should be used.
465465
*/
466-
private boolean copyFromLocalPerfomance;
466+
private boolean optimizedCopyFromLocal;
467467

468468
/** Add any deprecated keys. */
469469
@SuppressWarnings("deprecation")
@@ -691,8 +691,9 @@ public void initialize(URI name, Configuration originalConf)
691691
AWS_S3_VECTOR_ACTIVE_RANGE_READS, DEFAULT_AWS_S3_VECTOR_ACTIVE_RANGE_READS, 1);
692692
vectoredIOContext = populateVectoredIOContext(conf);
693693
scheme = (this.uri != null && this.uri.getScheme() != null) ? this.uri.getScheme() : FS_S3A;
694-
copyFromLocalPerfomance = conf.getBoolean(COPY_FROM_LOCAL_ENABLED,
695-
COPY_FROM_LOCAL_ENABLED_DEFAULT);
694+
optimizedCopyFromLocal = conf.getBoolean(OPTIMIZED_COPY_FROM_LOCAL,
695+
OPTIMIZED_COPY_FROM_LOCAL_DEFAULT);
696+
LOG.debug("Using optimized copyFromLocal implementation: {}", optimizedCopyFromLocal);
696697
} catch (SdkException e) {
697698
// amazon client exception: stop all services then throw the translation
698699
cleanupWithLogger(LOG, span);
@@ -4026,7 +4027,7 @@ private boolean s3Exists(final Path path, final Set<StatusProbeEnum> probes)
40264027
* the given dst name.
40274028
*
40284029
* This version doesn't need to create a temporary file to calculate the md5.
4029-
* If {@link Constants#COPY_FROM_LOCAL_ENABLED} is set to false,
4030+
* If {@link Constants#OPTIMIZED_COPY_FROM_LOCAL} is set to false,
40304031
* the superclass implementation is used.
40314032
*
40324033
* @param delSrc whether to delete the src
@@ -4044,17 +4045,20 @@ public void copyFromLocalFile(boolean delSrc, boolean overwrite, Path src,
40444045
checkNotClosed();
40454046
LOG.debug("Copying local file from {} to {} (delSrc={} overwrite={}",
40464047
src, dst, delSrc, overwrite);
4047-
if (copyFromLocalPerfomance) {
4048-
trackDurationAndSpan(INVOCATION_COPY_FROM_LOCAL_FILE, dst,
4049-
() -> new CopyFromLocalOperation(
4048+
if (optimizedCopyFromLocal) {
4049+
trackDurationAndSpan(INVOCATION_COPY_FROM_LOCAL_FILE, dst, () ->
4050+
new CopyFromLocalOperation(
40504051
createStoreContext(),
40514052
src,
40524053
dst,
40534054
delSrc,
40544055
overwrite,
4055-
createCopyFromLocalCallbacks()).execute());
4056+
createCopyFromLocalCallbacks(getActiveAuditSpan()))
4057+
.execute());
40564058
} else {
4057-
// call the superclass, but still count statistics
4059+
// call the superclass, but still count statistics.
4060+
// there is no overall span here, as each FS API call will
4061+
// be in its own span.
40584062
LOG.debug("Using base copyFromLocalFile implementation");
40594063
trackDurationAndSpan(INVOCATION_COPY_FROM_LOCAL_FILE, dst, () -> {
40604064
super.copyFromLocalFile(delSrc, overwrite, src, dst);
@@ -4063,17 +4067,29 @@ public void copyFromLocalFile(boolean delSrc, boolean overwrite, Path src,
40634067
}
40644068
}
40654069

4070+
/**
4071+
* Create the CopyFromLocalCallbacks;
4072+
* protected to assist in mocking
4073+
* @param span audit span.
4074+
* @return the callbacks
4075+
* @throws IOException failure to get the local fs.
4076+
*/
40664077
protected CopyFromLocalOperation.CopyFromLocalOperationCallbacks
4067-
createCopyFromLocalCallbacks() throws IOException {
4078+
createCopyFromLocalCallbacks(final AuditSpanS3A span) throws IOException {
40684079
LocalFileSystem local = getLocal(getConf());
4069-
return new CopyFromLocalCallbacksImpl(local);
4080+
return new CopyFromLocalCallbacksImpl(span, local);
40704081
}
40714082

40724083
protected final class CopyFromLocalCallbacksImpl implements
40734084
CopyFromLocalOperation.CopyFromLocalOperationCallbacks {
4085+
4086+
/** Span to use for all operations. */
4087+
private final AuditSpanS3A span;
40744088
private final LocalFileSystem local;
40754089

4076-
private CopyFromLocalCallbacksImpl(LocalFileSystem local) {
4090+
private CopyFromLocalCallbacksImpl(final AuditSpanS3A span,
4091+
LocalFileSystem local) {
4092+
this.span = span;
40774093
this.local = local;
40784094
}
40794095

@@ -4095,21 +4111,18 @@ public boolean deleteLocal(Path path, boolean recursive) throws IOException {
40954111

40964112
@Override
40974113
public void copyLocalFileFromTo(File file, Path from, Path to) throws IOException {
4098-
trackDurationAndSpan(
4099-
OBJECT_PUT_REQUESTS,
4100-
to,
4101-
() -> {
4102-
final String key = pathToKey(to);
4103-
Progressable progress = null;
4104-
PutObjectRequest.Builder putObjectRequestBuilder =
4105-
newPutObjectRequestBuilder(key, file.length(), false);
4106-
final String d = to.toString();
4107-
S3AFileSystem.this.invoker.retry("putObject(" + d + ")", d, true,
4108-
() -> executePut(putObjectRequestBuilder.build(), progress, putOptionsForPath(to),
4109-
file));
4110-
4114+
// the duration of the put is measured, but the active span is the
4115+
// constructor-supplied one -this ensures all audit log events are grouped correctly
4116+
span.activate();
4117+
trackDuration(getDurationTrackerFactory(), OBJECT_PUT_REQUESTS.getSymbol(), () -> {
4118+
final String key = pathToKey(to);
4119+
PutObjectRequest.Builder putObjectRequestBuilder =
4120+
newPutObjectRequestBuilder(key, file.length(), false);
4121+
final String dest = to.toString();
4122+
S3AFileSystem.this.invoker.retry("putObject(" + dest + ")", dest, true, () ->
4123+
executePut(putObjectRequestBuilder.build(), null, putOptionsForPath(to), file));
41114124
return null;
4112-
});
4125+
});
41134126
}
41144127

41154128
@Override
@@ -5370,6 +5383,10 @@ public boolean hasPathCapability(final Path path, final String capability)
53705383
case FS_S3A_CREATE_HEADER:
53715384
return true;
53725385

5386+
// is the optimized copy from local enabled.
5387+
case OPTIMIZED_COPY_FROM_LOCAL:
5388+
return optimizedCopyFromLocal;
5389+
53735390
default:
53745391
return super.hasPathCapability(p, cap);
53755392
}

hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/troubleshooting_s3a.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2050,6 +2050,6 @@ There are some switches which can be set to enable/disable features and assist
20502050
in isolating problems and at least make them "go away".
20512051

20522052

2053-
| Key | Default | Action |
2054-
|----------------------------------|---------|----------------------------------------------------------------------------------------------------------|
2055-
| `fs.s3a.copy.from.local.enabled` | `true` | [HADOOP-18925](https://issues.apache.org/jira/browse/HADOOP-18925) enable/disable CopyFromLocalOperation |
2053+
| Key | Default | Action |
2054+
|------|---------|----------|
2055+
| `fs.s3a.optimized.copy.from.local.enabled` | `true` | [HADOOP-18925](https://issues.apache.org/jira/browse/HADOOP-18925) enable/disable CopyFromLocalOperation. Also a path capability. |

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3ACopyFromLocalFile.java

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,14 @@
2828
import org.apache.hadoop.fs.contract.s3a.S3AContract;
2929

3030
import org.apache.hadoop.fs.Path;
31+
32+
import org.assertj.core.api.Assertions;
3133
import org.junit.Test;
3234
import org.junit.runner.RunWith;
3335
import org.junit.runners.Parameterized;
3436

35-
import static org.apache.hadoop.fs.s3a.Constants.COPY_FROM_LOCAL_ENABLED;
37+
import static org.apache.hadoop.fs.s3a.Constants.OPTIMIZED_COPY_FROM_LOCAL;
38+
import static org.apache.hadoop.fs.s3a.S3ATestUtils.disableFilesystemCaching;
3639
import static org.apache.hadoop.fs.s3a.S3ATestUtils.getTestBucketName;
3740
import static org.apache.hadoop.fs.s3a.S3ATestUtils.removeBaseAndBucketOverrides;
3841
import static org.apache.hadoop.test.LambdaTestUtils.intercept;
@@ -66,8 +69,9 @@ protected Configuration createConfiguration() {
6669
final Configuration conf = super.createConfiguration();
6770

6871
removeBaseAndBucketOverrides(getTestBucketName(conf), conf,
69-
COPY_FROM_LOCAL_ENABLED);
70-
conf.setBoolean(COPY_FROM_LOCAL_ENABLED, enabled);
72+
OPTIMIZED_COPY_FROM_LOCAL);
73+
conf.setBoolean(OPTIMIZED_COPY_FROM_LOCAL, enabled);
74+
disableFilesystemCaching(conf);
7175
return conf;
7276
}
7377

@@ -76,6 +80,15 @@ protected AbstractFSContract createContract(Configuration conf) {
7680
return new S3AContract(conf);
7781
}
7882

83+
@Test
84+
public void testOptionPropagation() throws Throwable {
85+
Assertions.assertThat(getFileSystem().hasPathCapability(new Path("/"),
86+
OPTIMIZED_COPY_FROM_LOCAL))
87+
.describedAs("path capability of %s", OPTIMIZED_COPY_FROM_LOCAL)
88+
.isEqualTo(enabled);
89+
90+
}
91+
7992
@Test
8093
public void testLocalFilesOnly() throws Throwable {
8194
describe("Copying into other file systems must fail");

0 commit comments

Comments
 (0)