dannycjones commented on code in PR #5548:
URL: https://github.com/apache/hadoop/pull/5548#discussion_r1166484677
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java:
##########
@@ -2590,6 +2591,25 @@ protected DurationTrackerFactory
getDurationTrackerFactory() {
: null;
}
+ /**
+ * Given a possibly null duration tracker factory, return a non-null
+ * one for use in tracking durations.
+ *
+ * @param factory factory.
+ * @return a non-null factory.
+ */
+ protected DurationTrackerFactory nonNullDurationTrackerFactory(
+ DurationTrackerFactory factory) {
+ DurationTrackerFactory f = factory;
+ if (f == null) {
+ f = getDurationTrackerFactory();
+ if (f == null) {
+ f = emptyStatisticsStore();
+ }
+ }
+ return f;
+ }
Review Comment:
Why do we add this non-null method if we use this method to create duration
trackers anyway?
https://github.com/apache/hadoop/blob/f6fa5bd1aa085a4d22f3450b545bb70063da9f51/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/statistics/impl/IOStatisticsBinding.java#L665-L678
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AMiscOperations.java:
##########
@@ -114,7 +114,7 @@ public void testPutObjectDirect() throws Throwable {
new ByteArrayInputStream("PUT".getBytes()),
metadata);
LambdaTestUtils.intercept(IllegalStateException.class,
- () -> fs.putObjectDirect(put, PutObjectOptions.keepingDirs()));
+ () -> fs.putObjectDirect(put, PutObjectOptions.keepingDirs(), null));
Review Comment:
Why not grab the duration tracker from the FS here?
```suggestion
() -> fs.putObjectDirect(put, PutObjectOptions.keepingDirs(),
fs.getDurationTrackerFactory()));
```
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/audit/AWSRequestAnalyzer.java:
##########
@@ -216,6 +216,18 @@ private RequestInfo writing(final String verb,
|| request instanceof CompleteMultipartUploadRequest
|| request instanceof GetBucketLocationRequest;
}
+ /**
+ * Predicate which returns true if the request is part of the
+ * multipart upload API -and which therefore must be rejected
+ * if multipart upload is disabled.
+ * @param request request
+ * @return true if the transfer manager creates them.
+ */
+ public static boolean isRequestMultipartIO(final Object request) {
+ return request instanceof CopyPartRequest
+ || request instanceof CompleteMultipartUploadRequest
+ || request instanceof InitiateMultipartUploadRequest;
+ }
Review Comment:
I think we're missing `UploadPartRequest` for any new parts being uploaded.
https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/services/s3/model/UploadPartRequest.html
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/scale/AbstractSTestS3AHugeFiles.java:
##########
@@ -192,13 +201,7 @@ public void test_010_CreateHugeFile() throws IOException {
true,
uploadBlockSize,
progress)) {
- try {
- streamStatistics = getOutputStreamStatistics(out);
- } catch (ClassCastException e) {
- LOG.info("Wrapped output stream is not block stream: {}",
- out.getWrappedStream());
- streamStatistics = null;
- }
+ streamStatistics = getOutputStreamStatistics(out);
Review Comment:
why do we drop this? it wasn't right in the first place, so we should fail
if we hit it?
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/audit/TestAuditIntegration.java:
##########
@@ -67,6 +71,22 @@ public void testExceptionTranslation() throws Throwable {
});
}
+ /**
+ * UnsupportedRequest mapping and fail fast outcome.
+ */
+ @Test
+ public void testUnsupportedExceptionTranslation() throws Throwable {
+ final UnsupportedRequestException ex =
intercept(UnsupportedRequestException.class, () -> {
+ throw translateException("test", "/",
+ new AuditOperationRejectedException("not supported"));
+ });
+ final S3ARetryPolicy retryPolicy = new S3ARetryPolicy(new
Configuration(false));
+ final RetryPolicy.RetryAction action = retryPolicy.shouldRetry(ex, 1, 1,
true);
Review Comment:
This should return "fail" request action with 0 for "retries" and
"failovers", no?
```suggestion
final RetryPolicy.RetryAction action = retryPolicy.shouldRetry(ex, 0, 0,
true);
```
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/scale/AbstractSTestS3AHugeFiles.java:
##########
@@ -119,6 +119,14 @@ protected Configuration createScaleConfiguration() {
DEFAULT_HUGE_PARTITION_SIZE);
assertTrue("Partition size too small: " + partitionSize,
partitionSize >= MULTIPART_MIN_SIZE);
+ removeBaseAndBucketOverrides(conf,
+ SOCKET_SEND_BUFFER,
+ SOCKET_RECV_BUFFER,
+ MIN_MULTIPART_THRESHOLD,
+ MULTIPART_SIZE,
+ USER_AGENT_PREFIX,
+ FAST_UPLOAD_BUFFER);
Review Comment:
random comment on maintainability of the tests in S3A
would a wrapper for `Configuration` make more sense, where this wrapper
would remove base/bucket overrides for any method you set?
This wrapper would just intercept `Configuration::set(String name, String
value, String source)`, with a body like:
```java
/** Wrapper for Configuration which drops any bucket overrides when setting
a configuration **/
public TestConfiguration extends Configuration {
private Configuration innerConfiguration;
public TestConfiguration(Configuration conf) {
this.innerConfiguration = conf;
}
public void set(String name, String value, String source) {
this.removeBaseAndBucketOverrides(name);
this.innerConfiguration.set(name, value, source);
}
}
```
would be a mess to update all existing tests of course
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/WriteOperationHelper.java:
##########
@@ -590,10 +593,9 @@ public void uploadObject(PutObjectRequest putObjectRequest,
PutObjectOptions putOptions)
throws IOException {
- retry("Writing Object",
- putObjectRequest.getKey(), true,
- withinAuditSpan(getAuditSpan(), () ->
- owner.putObjectDirect(putObjectRequest, putOptions)));
+ // the transfer manager is not involved; instead it is directly
+ // PUT.
+ putObject(putObjectRequest, putOptions, null);
Review Comment:
Why the change? Is it a change?
The JavaDoc is a bit confusing. It suggests this method might use transfer
manager, but we don't. Looking at `WriteOperations`, its JavaDoc says "PUT an
object directly (i.e. not via the transfer manager).", which conflicts with
this methods javadoc.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
