Repository navigation
Azure multi read options - #15630
Merged
Merged
Azure multi read options#15630
Conversation
added 30 commits
October 18, 2023 17:16
zachjsh
reviewed
Jan 23, 2024
zachjsh
reviewed
Jan 23, 2024
zachjsh
reviewed
Jan 23, 2024
|
|
||
| if (currentUri.getScheme().equals(AzureStorageAccountInputSource.SCHEME)) { | ||
| CloudObjectLocation cloudObjectLocation = new CloudObjectLocation(currentUri); | ||
| Pair<String, String> containerInfo = AzureStorageAccountInputSource.getContainerAndPathFromObjectLocation(cloudObjectLocation); |
Contributor
There was a problem hiding this comment.
ditto about maybe the need to validate the pair returned?
Contributor
Author
There was a problem hiding this comment.
what do u think we should validate about it?
Contributor
There was a problem hiding this comment.
What is either in the pair come back empty or null, would that cause unexpected / unhandled error elsewhere?
Contributor
Author
There was a problem hiding this comment.
i added a check in the function itself so it doesn't throw a index error on a bad input spec now. basically what happens now is the task will eventually fail b/c it fails to read the bad input file which i think is okay (the error from azure is pretty clear)
zachjsh
reviewed
Jan 23, 2024
317brian
approved these changes
Jan 24, 2024
317brian
left a comment
317brian
left a comment
Contributor
There was a problem hiding this comment.
Some small nits in the docs pages but nothing major. Thanks!
Co-authored-by: 317brian <53799971+317brian@users.noreply.github.com>
Co-authored-by: 317brian <53799971+317brian@users.noreply.github.com>
Co-authored-by: 317brian <53799971+317brian@users.noreply.github.com>
Co-authored-by: 317brian <53799971+317brian@users.noreply.github.com>
Co-authored-by: 317brian <53799971+317brian@users.noreply.github.com>
Co-authored-by: 317brian <53799971+317brian@users.noreply.github.com>
Co-authored-by: 317brian <53799971+317brian@users.noreply.github.com>
Co-authored-by: 317brian <53799971+317brian@users.noreply.github.com>
Co-authored-by: 317brian <53799971+317brian@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
added 2 commits
January 25, 2024 10:05
riovic918data
pushed a commit
to riovic918data/druid
that referenced
this pull request
Jun 12, 2026
* Include new dependencies * Mostly implemented * More azure fixes * Tests passing * Unit tests running * Test running after removing storage exception * Happy with coverage now * Add more tests * fix client factory * cleanup from testing * Remove old client * update docs * Exclude from spellcheck * Add licenses * Fix identity version * Save work * Add azure clients * add licenses * typos * Add dependencies * Exception is not thrown * Fix intellij check * Don't need to override * specify length * urldecode * encode path * Fix checks * Revert urlencode changes * Urlencode with azure library * Update docs/development/extensions-core/azure.md Co-authored-by: Abhishek Agarwal <1477457+abhishekagarwal87@users.noreply.github.com> * PR changes * Update docs/development/extensions-core/azure.md Co-authored-by: 317brian <53799971+317brian@users.noreply.github.com> * Add config for multiple storage accounts * Deprecate AzureTaskLogsConfig.maxRetries * Clean up azure retry block * logic update to reuse clients * fix comments * Create container conditionally * Fix key auth * save work * Fix unit tests * Revert old azure input type * Separate input source * save work * Add support for app registrations * Fix unit tests * clean up spacing * Add coverage * fixes from testing * cleanup some caching behavior * Add docs * Fix spelling issues * fix more spelling errors' * Fix intellij inspections * add simple changes from pr * save work on fixing bug * Fix unit tests * Add more testing * Fix unit test * Add tests * Add annotation for azureStorage * Fix up docs * Add comment for list method * Fix tests * Remove uneeded toString * Update docs/ingestion/input-sources.md Co-authored-by: 317brian <53799971+317brian@users.noreply.github.com> * Update docs/ingestion/input-sources.md Co-authored-by: 317brian <53799971+317brian@users.noreply.github.com> * Update docs/ingestion/input-sources.md Co-authored-by: 317brian <53799971+317brian@users.noreply.github.com> * Update docs/ingestion/input-sources.md Co-authored-by: 317brian <53799971+317brian@users.noreply.github.com> * Update docs/ingestion/input-sources.md Co-authored-by: 317brian <53799971+317brian@users.noreply.github.com> * Update docs/ingestion/input-sources.md Co-authored-by: 317brian <53799971+317brian@users.noreply.github.com> * Update docs/ingestion/input-sources.md Co-authored-by: 317brian <53799971+317brian@users.noreply.github.com> * Update docs/ingestion/input-sources.md Co-authored-by: 317brian <53799971+317brian@users.noreply.github.com> * Update docs/ingestion/input-sources.md Co-authored-by: 317brian <53799971+317brian@users.noreply.github.com> * PR changes * fix injection of StorageConnector * Fix checkstyle * clean up unit tests * More pr fixes --------- Co-authored-by: Abhishek Agarwal <1477457+abhishekagarwal87@users.noreply.github.com> Co-authored-by: 317brian <53799971+317brian@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Currently the "azure" input source schema only supports ingesting files that are stored in the Azure Storage Account specified in
druid.azure.account. To support ingesting data from different storage accounts, add a new "azureStorage" input source schema for azure with a slightly different spec.Fixed the bug ...
Renamed the class ...
Added a forbidden-apis entry ...
I would have preferred to keep using the regular AzureInputSource class but that class assumes CloudObjectLocation.bucket to be the container of the file and CloudObjectLocation.path to be the path the file within the bucket. I couldn't think of a way to keep the behavior backwards compatible with existing ingestion specs and support multiple storage accounts.
One idea i had was to introduce a "storageAccount" field directly in AzureInputSource (instead of in CloudObjectLocation), then use that field when creating AzureStorage classes. I think this would work but it also would mean that the behavior of azure ingestion is different from other types of ingestion (by having the storage account not in the ingestion uri) and this could cause problems down the line.
Since the AzureStorage instance is a abstraction over Azure Blob Storage clients, I thought it made sense to create a instance of it per Azure ingestion spec, since we do something similar with S3 (create a s3 client for each s3 ingestion spec).
It would have also been possible to pass the AzureInpuSourceConfig to the relevant functions in AzureStorage and generate different clients but I thought this would have been confusing.
I added support for key, sas token, and app registration authentication when ingesting from storage accounts. Managed/Workload identity auth can also work by not specifying properties and making sure the identity the cluster is deployed with can access the external storage account.
Release note
Support Azure ingestion from multiple Storage Accounts.
Key changed/added classes in this PR
AzureStorageAccountInputSourceAzureInpuSourceConfigAzureClientFactoryAzureStorageAzureEntityThis PR has: