ESQL: Data sources: Azure plugin#143236
Conversation
This adds support for the Azure Blob Storage.
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Hi @bpintea, I've created a changelog YAML for you. |
costin
left a comment
There was a problem hiding this comment.
LGTM. Left a few comments; in particular the close() one might address the thread leakage.
| public StorageIterator listObjects(StoragePath prefix, boolean recursive) throws IOException { | ||
| validateAzureScheme(prefix); | ||
| ParsedPath parsed = parsePathForListing(prefix); | ||
| String account = extractAccountFromHost(parsed.host); |
There was a problem hiding this comment.
close() only nulls the reference but never calls blobServiceClient.close(). BlobServiceClient uses reactor-netty event loops under the hood, so this leaks threads and connections. GCS calls storage.close(), S3 calls s3Client.close(). Should snapshot the reference, null it, then close it in a try/catch like GCS does.
| || (sasToken != null && sasToken.isEmpty() == false); | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
This is a Java record, which auto-generates equals() and hashCode() from all components. The custom implementations here are identical to what the compiler generates. Can just remove them.
| java.net.URL resourceUrl = AzureFixtureUtils.class.getResource(FIXTURES_RESOURCE_PATH); | ||
| if (resourceUrl == null) { | ||
| logger.warn("Fixtures resource path not found: {}", FIXTURES_RESOURCE_PATH); | ||
| return; |
There was a problem hiding this comment.
Nit: use imports instead of FQN for java.io.ByteArrayInputStream and java.net.URL. Codebase convention is to always import.
| private final StoragePath path; | ||
|
|
||
| private Long cachedLength; | ||
| private Instant cachedLastModified; |
There was a problem hiding this comment.
These cached fields are written in fetchMetadata() without synchronization or volatile. If a StorageObject is ever accessed from multiple threads there could be visibility issues. Worth either making them volatile or documenting that instances are single-threaded.
There was a problem hiding this comment.
Made them volatile, though the resolution of the resource needs to happen uni-threaded first and unlikely that'll change, but it's even safer now.
| @@ -0,0 +1,10 @@ | |||
| ALL-UNNAMED: | |||
There was a problem hiding this comment.
repository-azure scopes entitlements to specific modules (io.netty.common, reactor.core, etc.) rather than ALL-UNNAMED. Worth aligning with that pattern so entitlements are narrower.
The failure was caused by the esql-datasource-azure entitlement policy using module-scoped entitlements (io.netty.common, reactor.core, etc.) while the plugin layer only exposes ALL-UNNAMED.
This adds support for the Azure Blob Storage. 🤖 Developed AI-assisted.
This adds support for the Azure Blob Storage.
🤖 Developed AI-assisted.