AWS: OSS S3 Signer Updates#6835
Conversation
| } | ||
|
|
||
| private static SimpleModule initModule() { | ||
| public static SimpleModule initModule() { |
There was a problem hiding this comment.
made public here and above so that it can be easier reused
| if (!isInitialized) { | ||
| synchronized (S3ObjectMapper.class) { |
There was a problem hiding this comment.
Something I missed earlier but it's minor, generally for these kinds of initializations I think it's more clear to have an AtomicReference() and use the compareAndSet method if the instance is null.
There was a problem hiding this comment.
that is actually a good idea and I agree that we can improve this area. I opened #6857 to address that
9315761 to
032caf7
Compare
| * A token supplier that takes precedence over {@link S3V4RestSignerClient#token()} if it's set. | ||
| */ | ||
| @Nullable | ||
| public abstract Supplier<String> tokenSupplier(); |
There was a problem hiding this comment.
I feel like it's unnecessary to have both token() and tokenSupplier(). I think we should just make token() be a supplier and then the default can always be properties().get(OAuth2Properties.TOKEN. Having duplicates with precedence considerations just makes it more complicated.
There was a problem hiding this comment.
that makes a lot of sense. I've made those changes and pushed
032caf7 to
28b4ee2
Compare
No description provided.