Skip to content
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

feature/INTERLOK-3579 - Interlok AWS - use the AWS SDK v2 #698

Open
wants to merge 49 commits into
base: develop
Choose a base branch
from

Conversation

albinoloverats
Copy link

@albinoloverats albinoloverats commented Oct 6, 2021

Motivation

Currently use AWS SDK 1 for Java, however it's worth getting started on using AWS SDK 2. I had created a new project but it was decided to keep everything AWS together.

NB There are lots of changes: https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/home.html

including their package names, class names, method names, ways of doing things etc. Including features that are not yet present: S3 Transfer manager, S3 Encryption Client, DynamoDB document APIs, DynamoDB Encryption Client and SQS Client-side Buffering; plus Progress Listeners.

Modification

Pretty much everything.

PR Checklist

  • been self-reviewed.
  • Added javadocs for most classes and all non-trivial methods
  • Added supporting annotations (like @XStreamAlias / @ComponentProfile)
  • Added DefaultValue annotation when there is a default value (e.g. @DefaultValue('true'))
  • Added validation annotation (@NotNull...) when required and add @Valid when nested class has some validation
  • Checked that @NotNull and @notblank annotations have a meaningful message when appropriate.
  • Checked that new 3rd party dependencies are appropriately licensed
  • Added comments explaining the "why" and the intent of the code wherever it would not be obvious for an unfamiliar reader
  • Added unit tests or modified existing tests to cover new code paths
  • Tested new/updated components in the UI and at runtime in an Interlok instance
  • Reviewed java class members so that missing annotations are added (InputFieldDefault/ComponentProfile etc)
  • Checked that javadoc generation doesn't report errors
  • Checked the display of the component in the UI

Result

Most things should be the same, although I've noticed that there are going to be config differences around any authentication.

Testing

You will need an existing AWS/Adapter config that can be migrated to the new AWS2 components. This config will upload a file to S3 using both v1 and v2 of the AWS SDK. (You will need to supply the bucket, as well as the access token and secret key.)

Ashley Morgan Anderson added 30 commits September 17, 2021 09:48
Some classes are now the new hotness of AWS SDK 2 in the
interlok-aws2-common project, but many things are still broken.
There doesn't seem to be anything sensible that replaces
QueueBufferConfig
Most of the tests have been updated, although a few are (currently)
commented out because it's not immediately clear which direction to go
with them.
Ashley Morgan Anderson and others added 16 commits October 5, 2021 13:58
I think there is still more to do with S3, but another pair of eyes
might be useful at this time.
Build fails on Github (but annoyingly not locally) in
ApacheSigningInterceptor#build() and the only likely issue I can see is
with Region#of(String), so we'll see...
Still trying totrack down why everything works locally but not on
Github; the search continues...
The common reason for failure seems to be missing credentials.
@codecov
Copy link

codecov bot commented Oct 11, 2021

Codecov Report

Merging #698 (8e6a023) into develop (e985097) will decrease coverage by 3.98%.
The diff coverage is 90.85%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop     #698      +/-   ##
=============================================
- Coverage      99.01%   95.03%   -3.99%     
- Complexity       508      911     +403     
=============================================
  Files             81      154      +73     
  Lines           1421     2678    +1257     
  Branches          64      118      +54     
=============================================
+ Hits            1407     2545    +1138     
- Misses            11      118     +107     
- Partials           3       15      +12     
Impacted Files Coverage Δ
...om/adaptris/aws2/s3/meta/S3ContentDisposition.java 0.00% <0.00%> (ø)
...a/com/adaptris/aws2/s3/meta/S3ContentEncoding.java 0.00% <0.00%> (ø)
...a/com/adaptris/aws2/s3/meta/S3ContentLanguage.java 0.00% <0.00%> (ø)
.../java/com/adaptris/aws2/s3/meta/S3ContentType.java 0.00% <0.00%> (ø)
.../adaptris/aws2/s3/meta/S3ExpirationTimeRuleId.java 0.00% <0.00%> (ø)
...n/java/com/adaptris/aws2/s3/DownloadOperation.java 12.90% <12.90%> (ø)
...va/com/adaptris/aws2/s3/meta/S3ObjectMetadata.java 50.00% <50.00%> (ø)
...a/com/adaptris/aws2/s3/meta/S3HttpExpiresDate.java 57.14% <57.14%> (ø)
...com/adaptris/aws2/s3/CheckFileExistsOperation.java 72.72% <72.72%> (ø)
.../java/com/adaptris/aws2/s3/retry/S3RetryStore.java 74.50% <74.50%> (ø)
... and 66 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e985097...8e6a023. Read the comment docs.

@quotidian-ennui
Copy link
Member

@albinoloverats -> I bumped mockito to 4.0.0, this might break some of your tests, probably worth merging develop into this.

@aaron-mcgrath-adp
Copy link
Member

We should document what v1 and v2 supports and doesn't support, with a few config examples.

@Setter
private String secretKey;

public AWSKeysAuthentication() {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use @NoArgsConstructor instead?

public class ClientConfigurationBuilder {

private static transient Logger log = LoggerFactory.getLogger(ClientConfigurationBuilder.class);
//
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this? Do you see it being useful soon?

@Setter
private String signingRegion;

public CustomEndpoint() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, maybe we should use lombok @NoArgsConstructor everytime we have an empty no args constructor?

@quotidian-ennui
Copy link
Member

  • @since tags in javadocs are wrong
  • @config tags in javadocs are wrong (yes, they're wrong in the aws v1sdk variant as well).
  • since param in ComponentProfile where they are present are wrong.
  • Why is there a deprecated annotation on a brand new classes...
$ find . -name "*.java" -exec grep -il "deprecate" {} \; | grep aws2
./interlok-aws2-s3/src/main/java/com/adaptris/aws2/s3/S3OperationImpl.java
  • jacksonVersion isn't up-to-date on aws2-*, though we could let dependabot sort that out.
  • pom file github location is wrong for all aws2-*
  • Does interlok-aws2-sqs really require com.amazonaws:amazon-sqs-java-messaging-lib:1.0.8 since this pulls in the AWS SDK v1
    • and if it does then you need to review that with more purpose.
  • What's the impact of having the "same name" for all the classes in aws2... Not sure if there is one
    • but AmazonSQSConsumerMonitorMBean.class.getSimpleName() is going to be the same for both aws / aws2 which may break the UI?
    • recommended = {AmazonS3Connection.class} in componentProfile where you have both interlok-aws-s3 && interlok-aws2-s3 available?

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.

4 participants