OAK-9722 Allow replacing default Azure SAS token generator#519
OAK-9722 Allow replacing default Azure SAS token generator#519jelmini wants to merge 5 commits intoapache:trunkfrom
Conversation
|
@smiroslav @mattvryan Could you please review this PR? Thanks. |
OAK-9724 Handle duplicate functions gracefully in indexing
| azureBlobStoreBackend = new AzureBlobStoreBackend(); | ||
| if (null != properties) { | ||
| azureBlobStoreBackend.setProperties(properties); | ||
| azureBlobStoreBackend.setSasTokenGenerator(sasTokenGenerator); |
There was a problem hiding this comment.
This doesn't look right to me. Why set the sasTokenGenerator only when there are properties?
There was a problem hiding this comment.
Right. It should be outside the if clause.
| import org.jetbrains.annotations.NotNull; | ||
| import org.jetbrains.annotations.Nullable; | ||
|
|
||
| public interface SasTokenGenerator { |
There was a problem hiding this comment.
I think this interface should have some basic documentation and the new sas package should have an explicit export version in a package-info.java file. I understand the interface is intended to be implemented by 3rd parties and therefore must have the export version managed explicitly.
There was a problem hiding this comment.
Yes, during tests I realised explicit export version should be added.
This PR should have been a draft PR, as the goal was to receive early feedback.
mreutegg
left a comment
mreutegg
left a comment
There was a problem hiding this comment.
This PR also contains changes unrelated to this issue under oak-lucene.
Hmm, weird. I have pushed a single commit with changes only to oak-blob-cloud-azure, but now this PR thinks other unrelated commits were added to it... I don't know how that could happen. |
4 participants
No description provided.