Skip to content

HADOOP-18073. Address review comments. #31

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

ahmarsuhail
Copy link
Owner

@ahmarsuhail ahmarsuhail commented Nov 25, 2022

  • Fixes (most) yetus errors
  • Reorders imports
  • Moves AWSCannedACL and AWSClientConfig to impl package
  • Removes com.amazonaws.services.s3.Headers import, and instead adds in a AWSHeader class which includes only the headers S3A uses.

.region(region)
.serviceConfiguration(serviceConfiguration);
S3Configuration serviceConfiguration =
S3Configuration.builder().pathStyleAccessEnabled(parameters.isPathStyleAccess())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the layout/indent change intended?

import org.apache.hadoop.util.Progressable;
import org.slf4j.Logger;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this supposed to go in the same block as software..?

@@ -30,6 +30,8 @@
import java.util.Map;
import java.util.concurrent.TimeUnit;

import software.amazon.awssdk.core.exception.SdkException;
import org.apache.hadoop.util.Preconditions;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why Hadoop here?

/**
* Common S3 HTTP header values used throughout the Amazon Web Services S3 Java client.
*/
public interface AWSHeaders {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this just be part of HeaderProcessing? If not, shouldn't it be in s3a.impl?

String ETAG = "ETag";
String LAST_MODIFIED = "Last-Modified";

/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are the constants and comments just copied from v1 Headers? Is that ok?

@ahmarsuhail ahmarsuhail merged commit 1ac2dd7 into feature-HADOOP-18073-s3a-sdk-upgrade Nov 28, 2022
ahmarsuhail added a commit that referenced this pull request Dec 9, 2022
addresses review comments + yetus errors

Co-authored-by: Ahmar Suhail <[email protected]>
ahmarsuhail added a commit that referenced this pull request Jan 18, 2023
addresses review comments + yetus errors

Co-authored-by: Ahmar Suhail <[email protected]>
ahmarsuhail added a commit that referenced this pull request May 15, 2023
addresses review comments + yetus errors

Co-authored-by: Ahmar Suhail <[email protected]>
ahmarsuhail added a commit that referenced this pull request May 16, 2023
addresses review comments + yetus errors

Co-authored-by: Ahmar Suhail <[email protected]>
ahmarsuhail added a commit that referenced this pull request May 17, 2023
addresses review comments + yetus errors

Co-authored-by: Ahmar Suhail <[email protected]>
ahmarsuhail added a commit that referenced this pull request Aug 17, 2023
See aws_sdk_v2_changelog.md for details.

Co-authored-by: Ahmar Suhail <[email protected]>
Co-authored-by: Alessandro Passaro <[email protected]>

HADOOP-18073. Address review comments. (#31)

addresses review comments + yetus errors

Co-authored-by: Ahmar Suhail <[email protected]>

Move MultiObjectDeleteException to impl

Reinstate old constants

Move TransferManager initialization to ClientFactory

Add unit tests for BlockingEnumeration

Add unit tests for SelectEventStreamPublisher

updates new providers in TestS3AAWSCredentialsProvider to V2

update GET range referrer header logic to V2

adds in unit check for bytes

HADOOP-18565. Complete outstanding items for the AWS SDK V2 upgrade. (apache#5421)

Changes include
* use bundled transfer manager
* adds transfer listener to upload
* adds support for custom signers
* don't set default endpoint
* removes v1 sdk bundle, only use core package
* implements region caching
+ many more

Note: spotbugs is warning about inconsistent
synchronization in accessing a new s3a FS field.
This will be fixed in a follow-up patch.

Contributed by Ahmar Suhail

fixes issues after rebase

Change-Id: I1d4b658979c725a88f6b54832bb7d106ffc2347d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants