steveloughran commented on a change in pull request #2148:
URL: https://github.com/apache/hadoop/pull/2148#discussion_r462484155
##########
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/OperationCallbacks.java
##########
@@ -40,7 +40,8 @@
import org.apache.hadoop.fs.s3a.s3guard.BulkOperationState;
/**
- * These are all the callbacks which the {@link RenameOperation}
+ * These are all the callbacks which the {@link RenameOperation},
Review comment:
not true any more though, not now there's a new listing interface, right?
##########
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Listing.java
##########
@@ -46,25 +56,27 @@
import java.util.Set;
import static org.apache.hadoop.fs.s3a.Constants.S3N_FOLDER_SUFFIX;
+import static org.apache.hadoop.fs.s3a.S3AUtils.ACCEPT_ALL;
import static org.apache.hadoop.fs.s3a.S3AUtils.createFileStatus;
+import static org.apache.hadoop.fs.s3a.S3AUtils.maybeAddTrailingSlash;
import static org.apache.hadoop.fs.s3a.S3AUtils.objectRepresentsDirectory;
import static org.apache.hadoop.fs.s3a.S3AUtils.stringify;
import static org.apache.hadoop.fs.s3a.S3AUtils.translateException;
+import static org.apache.hadoop.fs.s3a.auth.RoleModel.pathToKey;
/**
* Place for the S3A listing classes; keeps all the small classes under
control.
*/
@InterfaceAudience.Private
-public class Listing {
+public class Listing extends AbstractStoreOperation {
- private final S3AFileSystem owner;
private static final Logger LOG = S3AFileSystem.LOG;
static final FileStatusAcceptor ACCEPT_ALL_BUT_S3N =
new AcceptAllButS3nDirs();
- public Listing(S3AFileSystem owner) {
- this.owner = owner;
+ public Listing(StoreContext storeContext) {
Review comment:
have this take the context and the ListingOperationCallbacks, add a
method in S3AFS `createListing()` which can be used there and in
DumpS3GuardDynamoTable...that avoids having to extend the store context
##########
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
##########
@@ -4847,5 +4800,20 @@ public String getBucketLocation() throws IOException {
public Path makeQualified(final Path path) {
return S3AFileSystem.this.makeQualified(path);
}
+
Review comment:
Add to the ListingOperationCallbacks
##########
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/StoreContext.java
##########
@@ -116,6 +116,16 @@
*/
private ITtlTimeProvider timeProvider;
+ /**
Review comment:
I'd rather these were left out of the context and only explicitly passed
to those classes which explicitly needed the operations. That helps us stay in
control of what operations specific modules can perform
##########
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
##########
@@ -1599,6 +1603,36 @@ public boolean allowAuthoritative(final Path p) {
}
}
+ protected class ListingOperationCallbacksImpl implements
ListingOperationCallbacks {
+
+ @Override
+ @Retries.RetryRaw
+ public S3ListResult listObjects(
+ S3ListRequest request)
+ throws IOException {
+ return S3AFileSystem.this.listObjects(request);
+ }
+
+ @Override
+ @Retries.RetryRaw
Review comment:
nice touch
##########
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
##########
@@ -293,6 +294,9 @@
private final S3AFileSystem.OperationCallbacksImpl
operationCallbacks = new OperationCallbacksImpl();
+ private final S3AFileSystem.ListingOperationCallbacksImpl
Review comment:
Make the type of this field the interface for strictness
----------------------------------------------------------------
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]
