mehakmeet commented on a change in pull request #2778:
URL: https://github.com/apache/hadoop/pull/2778#discussion_r595978487
##########
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/auth/ITestCustomSigner.java
##########
@@ -128,24 +142,35 @@ private Configuration createTestConfig(String identifier)
{
conf.set(TEST_ID_KEY, identifier);
conf.set(TEST_REGION_KEY, regionName);
+ conf.set(Constants.ENDPOINT, endpoint);
+ // make absolutely sure there is no caching.
+ disableFilesystemCaching(conf);
return conf;
}
private String determineRegion(String bucketName) throws IOException {
- String region = getFileSystem().getBucketLocation(bucketName);
- return fixBucketRegion(region);
+ return getFileSystem().getBucketLocation(bucketName);
}
@Private
public static final class CustomSigner implements Signer {
- private static int invocationCount = 0;
+
+ private static AtomicInteger instantiationCount = new AtomicInteger(0);
Review comment:
could make this final
##########
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/DefaultS3ClientFactory.java
##########
@@ -70,22 +67,24 @@
LoggerFactory.getLogger(DefaultS3ClientFactory.class);
/**
- * Create the client.
+ * Create the client by preparing the AwsConf configuration
+ * and then invoking {@link #buildAmazonS3Client(AWSCredentialsProvider,
ClientConfiguration, S3ClientCreationParameters, String, boolean)}
Review comment:
this should be ```{@link #buildAmazonS3Client(ClientConfiguration,
S3ClientCreationParameters)}```?
##########
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/DefaultS3ClientFactory.java
##########
@@ -96,111 +95,62 @@ public AmazonS3 createS3Client(URI name,
conf.getBoolean(EXPERIMENTAL_AWS_INTERNAL_THROTTLING,
EXPERIMENTAL_AWS_INTERNAL_THROTTLING_DEFAULT));
- if (!StringUtils.isEmpty(userAgentSuffix)) {
- awsConf.setUserAgentSuffix(userAgentSuffix);
+ if (!StringUtils.isEmpty(parameters.getUserAgentSuffix())) {
+ awsConf.setUserAgentSuffix(parameters.getUserAgentSuffix());
}
- // optional metrics
- RequestMetricCollector metrics = statisticsFromAwsSdk != null
- ? new AwsStatisticsCollector(statisticsFromAwsSdk)
- : null;
- return newAmazonS3Client(
- credentials,
+ return buildAmazonS3Client(
awsConf,
- metrics,
- conf.getTrimmed(ENDPOINT, ""),
- conf.getBoolean(PATH_STYLE_ACCESS, false));
+ parameters);
}
/**
- * Create an {@link AmazonS3} client.
- * Override this to provide an extended version of the client
- * @param credentials credentials to use
- * @param awsConf AWS configuration
- * @param metrics metrics collector or null
- * @param endpoint endpoint string; may be ""
- * @param pathStyleAccess enable path style access?
- * @return new AmazonS3 client
- */
- protected AmazonS3 newAmazonS3Client(
- final AWSCredentialsProvider credentials,
- final ClientConfiguration awsConf,
- final RequestMetricCollector metrics,
- final String endpoint,
- final boolean pathStyleAccess) {
- if (metrics != null) {
- LOG.debug("Building S3 client using the SDK builder API");
- return buildAmazonS3Client(credentials, awsConf, metrics, endpoint,
- pathStyleAccess);
- } else {
- LOG.debug("Building S3 client using the SDK builder API");
- return classicAmazonS3Client(credentials, awsConf, endpoint,
- pathStyleAccess);
- }
- }
-
- /**
- * Use the (newer) Builder SDK to create a an AWS S3 client.
+ * Use the Builder API to create a an AWS S3 client.
Review comment:
nit: ~a~
##########
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/auth/ITestCustomSigner.java
##########
@@ -128,24 +142,35 @@ private Configuration createTestConfig(String identifier)
{
conf.set(TEST_ID_KEY, identifier);
conf.set(TEST_REGION_KEY, regionName);
+ conf.set(Constants.ENDPOINT, endpoint);
+ // make absolutely sure there is no caching.
+ disableFilesystemCaching(conf);
return conf;
}
private String determineRegion(String bucketName) throws IOException {
- String region = getFileSystem().getBucketLocation(bucketName);
- return fixBucketRegion(region);
+ return getFileSystem().getBucketLocation(bucketName);
}
@Private
public static final class CustomSigner implements Signer {
- private static int invocationCount = 0;
+
+ private static AtomicInteger instantiationCount = new AtomicInteger(0);
+ private static AtomicInteger invocationCount = new AtomicInteger(0);
Review comment:
could make this final
##########
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/statistics/ITestAWSStatisticCollection.java
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.statistics;
+
+import org.junit.Test;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.s3a.AbstractS3ATestBase;
+import org.apache.hadoop.fs.s3a.S3AFileSystem;
+import org.apache.hadoop.fs.statistics.IOStatistics;
+
+import static org.apache.hadoop.fs.s3a.Constants.DEFAULT_ENDPOINT;
+import static org.apache.hadoop.fs.s3a.Constants.ENDPOINT;
+import static org.apache.hadoop.fs.s3a.S3ATestUtils.getLandsatCSVPath;
+import static org.apache.hadoop.fs.s3a.Statistic.STORE_IO_REQUEST;
+import static
org.apache.hadoop.fs.statistics.IOStatisticAssertions.assertThatStatisticCounter;
+
+/**
+ * Verify that AWS SDK statistics are wired up.
+ * This test tries to read data from US-east-1 and us-west-2 buckets
+ * so as to be confident that the nuances of region mapping
+ * are handed correctly (HADOOP-13551).
+ * The statistics are probed to verify that the wiring up is complete.
+ */
+public class ITestAWSStatisticCollection extends AbstractS3ATestBase {
+
+ private static final Path COMMON_CRAWL_PATH
+ = new Path("s3a://osm-pds/planet/planet-latest.orc");
+
+ @Test
+ public void testLandsatStatistics() throws Throwable {
+ final Configuration conf = getConfiguration();
+ // skips the tests if the lansdat path isn't the default.
+ Path path = getLandsatCSVPath(conf);
+ conf.set(ENDPOINT, DEFAULT_ENDPOINT);
+ conf.unset("fs.s3a.bucket.landsat-pds.endpoint");
+
+ try (S3AFileSystem fs = (S3AFileSystem) path.getFileSystem(conf)) {
+ fs.getObjectMetadata(path);
+ IOStatistics iostats = fs.getIOStatistics();
+ assertThatStatisticCounter(iostats,
+ STORE_IO_REQUEST.getSymbol())
+ .isGreaterThanOrEqualTo(1);
+ }
+ }
+
+ @Test
+ public void testCommonCrawlStatistics() throws Throwable {
+ final Configuration conf = getConfiguration();
+ // skips the tests if the lansdat path isn't the default.
+ Path landsatPath = getLandsatCSVPath(conf);
+
+ Path path = COMMON_CRAWL_PATH;
+ conf.set(ENDPOINT, "");
Review comment:
could use DEFAULT_ENDPOINT here.
##########
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/statistics/ITestAWSStatisticCollection.java
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.statistics;
+
+import org.junit.Test;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.s3a.AbstractS3ATestBase;
+import org.apache.hadoop.fs.s3a.S3AFileSystem;
+import org.apache.hadoop.fs.statistics.IOStatistics;
+
+import static org.apache.hadoop.fs.s3a.Constants.ENDPOINT;
+import static org.apache.hadoop.fs.s3a.S3ATestUtils.getLandsatCSVPath;
+import static org.apache.hadoop.fs.s3a.Statistic.STORE_IO_REQUEST;
+import static
org.apache.hadoop.fs.statistics.IOStatisticAssertions.assertThatStatisticCounter;
+
+/**
+ * Verify that AWS SDK statistics are wired up.
+ * This test tries to read data from US-east-1 and us-west-2 buckets
+ * so as to be confident that the nuances of region mapping
+ * are handed correctly (HADOOP-13551).
+ * The statistics are probed to verify that the wiring up is complete.
+ */
+public class ITestAWSStatisticCollection extends AbstractS3ATestBase {
+
+ private static final Path COMMON_CRAWL_PATH
+ = new Path("s3a://osm-pds/planet/planet-latest.orc");
+
+ @Test
+ public void testLandsatStatistics() throws Throwable {
+ final Configuration conf = getConfiguration();
+ // skips the tests if the lansdat path isn't the default.
Review comment:
typo: "lansdat" -> "landsat". Below as well.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
