ProfileFileSupplier API changes, aggregating #3558

dave-fn · 2022-11-22T15:03:18Z

Motivation and Context

Update the ProfileFileSupplier API to include methods for aggregating multiple ProfileFile objects into a single ProfileFileSupplier. This will mimic how multiple ProfileFile can be aggregated into another ProfileFile object.
This change is in preparation for adding reloading functionality to DefaultCredentialsProvider.

Modifications

Revised 2 methods and added 4 methods to ProfileFileSupplier.

Testing

Added unit tests.

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

I have read the CONTRIBUTING document
Local run of mvn install succeeds
My code follows the code style of this project
My change requires a change to the Javadoc documentation
I have updated the Javadoc documentation accordingly
I have added tests to cover my changes
All new and existing tests passed
I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

I confirm that this pull request can be released under the Apache 2 license

L-Applin · 2022-11-22T17:20:00Z

core/profiles/src/main/java/software/amazon/awssdk/profiles/ProfileFileSupplier.java

+                currentValuesBySupplier.keySet().forEach(ProfileFileSupplier::close);
+            }
+
+            private boolean updateCurrentValue(ProfileFileSupplier supplier, Consumer<ProfileFile> action) {


maybe updateCurrentValue might not be the most accurate name for this method? It seems to also check if the supplier provided a modified ProfileFile.

Agree. I'll have to think hard to try to come up with an appropriate name.

Method has been removed as it was not needed as pointed out in other comment.

dagnir · 2022-11-22T18:01:48Z

.changes/next-release/feature-AWSSDKforJavav2-8c45016.json

+    "category": "AWS SDK for Java v2", 
+    "contributor": "", 
+    "type": "feature", 
+    "description": "Add methods to ProfileFileSupplier for aggregating"


Since we haven't merged the APIs to mainline yet, you don't need a changelog entry for this

dagnir · 2022-11-22T18:52:39Z

core/profiles/src/main/java/software/amazon/awssdk/profiles/ProfileFileSupplier.java

+                ProfileFile current = currentValuesBySupplier.put(supplier, next);
+                action.accept(next);
+
+                return !Objects.equals(next, current);


These checks seem redundant. The contract of the API is that ProfileFileSupplier#get should return the same object if it's not updated

Agree. Entire method has been removed.

L-Applin · 2022-11-29T18:50:24Z

core/profiles/src/main/java/software/amazon/awssdk/profiles/ProfileFileSupplier.java

+            private ProfileFile refreshAndGetCurrentAggregate(ProfileFile.Aggregator aggregator) {
+                ProfileFile next = aggregator.build();
+                if (isNewAggregateProfileFile(next)) {
+                    currentAggregateProfileFile = next;


Does that need to be thread safe? Do we need to use compare and set?

Used same pattern in other classes, for example ProfileFileRefresher. Interested to hear what @dagnir has to say, though.

Yeah I think we do; we want want to ensure that next does not change between lines 148 and 149. Good eye @L-Applin!

If the CAS fails, I think it's fine since that would mean a different thread already updated it.

Changed type of reference.

sonarqubecloud · 2022-11-30T15:31:34Z

SonarCloud Quality Gate failed.

2 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

88.6% Coverage
0.0% Duplication

* ProfileCredentialsProvider can now reload credentials when profile files change (#3487) * ProfileFile can update if disk changed, reload as new instance * ProfileCredentialsProvider reloads credentials if profile file has changes * Created class ProfileFileRefresher * Created ReloadingProfileCredentialsProvider; moved new logic in ProfileFile to ProfileFileRefresher * Fix ReloadingProfileCredentialsBehavior when missing ProfileFile Supplier or Predicate, and dealing with defaults * Consolidated ReloadingProfileCredentialsProvider functionality into ProfileCredentialsProvider * Fix behavior when dealing with defaults * Created ProfileFileSupplier interface; refactored * Misc fixes * Created package-private ProfileFileSupplierBuilder; ProfileFileSupplier now extends Supplier; Fixed Javadoc * Fixed unit tests for default credentials file * Removed ProfileFileSupplier.Builder interface * Code cleanup * ProfileFileSupplier API changes, aggregating (#3558) * Added methods for aggregating ProfileFile objects * Removed redundant logic, changelog entry * Removed redundant methods * Use compare and set to make thread safe * DefaultCredentialsProvider reload profile (#3580) * Misc fixes * Reload credentials by DefaultCredentialsProvider; pass supplier to InstanceProfileCredentialsProvider * Fix code alignment * Update public APIs to Supplier<ProfileFile> instead of ProfileFileSupplier (#3607) * ProfileTokenProvider Reload ProfileFile (#3608) * Updated ProfileTokenProvider * Updated tests, do not explicitly swallow exceptions * ProfileTokenProviderLoader can use Supplier<Profilefile> internally * Simplified ProfileTokenProviderLoader API; implemented synchronized block * Use synchronized block (#3646) * S3 support classes take Supplier<ProfileFile> (#3653) * S3 support classes take Supplier<ProfileFile> * Review comments * Presigners, other Support classes take Supplier<ProfileFile> (#3677) * Presigners, other Support classes take Supplier<ProfileFile> * Split new ProfileFile tests from existing parameterized tests * Improved tests readability * ProfileFile updates to BaseClientBuilderClass, other S3 classes (#3685) * Leverage SdkClientOption and SdkExecutionAttributes; fallback to simp… (#3692) * Leverage SdkClientOption and SdkExecutionAttributes; fallback to simple ProfileFile * Addressed review comments * Updated changelog entry (#3699) * Fixed review comments * Removed unnecessary logic * Deprecated SdkClientOption PROFILE_FILE * Deprecated SdkExecutionAttribute PROFILE_FILE * Updated changelog entry

Added methods for aggregating ProfileFile objects

a16a868

dave-fn requested a review from a team as a code owner November 22, 2022 15:03

L-Applin reviewed Nov 22, 2022

View reviewed changes

dagnir reviewed Nov 22, 2022

View reviewed changes

dave-fn added 2 commits November 22, 2022 12:31

Removed redundant logic, changelog entry

6442ee0

Removed redundant methods

50a7f25

L-Applin reviewed Nov 29, 2022

View reviewed changes

Use compare and set to make thread safe

b9c4225

dagnir approved these changes Dec 1, 2022

View reviewed changes

L-Applin approved these changes Dec 1, 2022

View reviewed changes

dave-fn merged commit 6b186a2 into aws:feature/master/credentials-reload Dec 1, 2022

dave-fn deleted the features/profile-file-supplier-aggregate branch December 1, 2022 14:22

ProfileFileSupplier API changes, aggregating #3558

ProfileFileSupplier API changes, aggregating #3558

Uh oh!

Conversation

dave-fn commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dave-fn Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Nov 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dave-fn commented Nov 22, 2022 •

edited

Loading

dave-fn Nov 30, 2022 •

edited

Loading