Fix Azure http client leak in Azure filesystem #21982
Conversation
findepi
added
the
no-release-notes
findepi
requested review from
electrum,
jklamer,
anusudarsan,
charlesjmorgan and
findinpath
findepi
force-pushed
the
findepi/fix-azure-http-client-leak-in-azure-filesystem-6d1547
branch
2 times, most recently
from
7ed766d
to
2fc79a0
Compare
| @@ -116,14 +118,18 @@ private boolean isHierarchicalNamespaceEnabled() | |||
| return fileSystemClient.getDirectoryClient("/").exists(); | |||
| } | |||
| catch (RuntimeException e) { | |||
| throw new IOException("Failed to check whether hierarchical namespaces is enabled for the storage account %s and container %s".formatted(account, containerName)); | |||
| throw new IOException("Failed to check whether hierarchical namespaces is enabled for the storage account %s and container %s" .formatted(account, containerName)); | |||
There was a problem hiding this comment.
thanks! for some reason intellij reformat added this for me.
will try to reset the airbase code style.
There was a problem hiding this comment.
i reimported airlift codestyle, but intellij still adds this space.
cc @trinodb/maintainers
...-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeAdlsConnectorSmokeTest.java
Show resolved
Hide resolved
| <dependency> | ||
| <groupId>io.trino</groupId> | ||
| <artifactId>trino-filesystem-azure</artifactId> | ||
| <scope>runtime</scope> |
There was a problem hiding this comment.
Could it be just test scoped? Same for the okhttp dependency above?
There was a problem hiding this comment.
it is needed by tests, but also needed transitively.
we need to use runtime scope in such case, which sadly disables dep check
| .configuration(clientOptions.getConfiguration()) | ||
| .connectionTimeout(clientOptions.getConnectTimeout()) | ||
| .writeTimeout(clientOptions.getWriteTimeout()) | ||
| .readTimeout(clientOptions.getReadTimeout()) | ||
| .connectionPool(new ConnectionPool(maximumConnectionPoolSize, | ||
| clientOptions.getConnectionIdleTimeout().toMillis(), TimeUnit.MILLISECONDS)).build(); |
There was a problem hiding this comment.
There may be potential for further config options in AzureFileSystemConfig
cc @anusudarsan
There was a problem hiding this comment.
ya we will have test internally with defaults and see if there is a need for it
findepi
force-pushed
the
findepi/fix-azure-http-client-leak-in-azure-filesystem-6d1547
branch
from
2fc79a0
to
ea712ce
Compare
findepi
deleted the
findepi/fix-azure-http-client-leak-in-azure-filesystem-6d1547
branch
Found by #21913