Add support for Azure Goverment storage #15523

nozjkoitop · 2023-12-08T11:08:17Z

Description

This pull request introduces a configurable Azure Storage Endpoint Suffix in the Druid Azure-Extensions.

Release note

Added support for Azure Government storage in Druid Azure-Extensions. This enhancement allows the Azure-Extensions to be compatible with different Azure storage types by updating the endpoint suffix from a hardcoded value to a configurable one.

Key changed/added classes in this PR

AzureAccountConfig
AzureUtils
AzureDataSegmentPuller
AzureStorageDruidModule

This PR has:

been self-reviewed.
added documentation for new or modified features or behaviors.
a release note entry in the PR description.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added or updated version, license, or notice information in licenses.yaml
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
added integration tests.
been tested in a test Druid cluster.

BartMiki

LGTM

abhishekagarwal87

thank you for the PR. I have left some comments.

abhishekagarwal87 · 2024-01-03T07:15:49Z

extensions-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureUtils.java

+   *                                  <code>"blob.core.chinacloudapi.cn"</code> or
+   *                                  <code>"blob.core.usgovcloudapi.net</code>"
+   */
+  public AzureUtils(String blobStorageEndpointSuffix)


since its a utils class, lets not add a constructor here.

abhishekagarwal87 · 2024-01-03T07:17:07Z

docs/development/extensions-core/azure.md

@@ -40,5 +40,6 @@ To use this Apache Druid extension, [include](../../configuration/extensions.md#
 |`druid.azure.protocol`|the protocol to use|http or https|https|
 |`druid.azure.maxTries`|Number of tries before canceling an Azure operation.| |3|
 |`druid.azure.maxListingLength`|maximum number of input files matching a given prefix to retrieve at a time| |1024|
+|`druid.azure.endpointSuffix`|the endpoint suffix to use.|Examples: `core.windows.net`, `core.usgovcloudapi.net`|`core.windows.net`|


can you add some more colour here? when would one want to override this? Pointing to external documentation is fine too.

added usecase with link to docs

extensions-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureUtils.java

abhishekagarwal87 · 2024-01-03T07:20:10Z

extensions-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureUtils.java


  // The azure storage hadoop access pattern is:
-  // wasb[s]://<containername>@<accountname>.blob.core.windows.net/<path>
+  // wasb[s]://<containername>@<accountname>.<blobStorageEndpointSuffix>/<path>


Suggested change

// wasb[s]://<containername>@<accountname>.<blobStorageEndpointSuffix>/<path>

// wasb[s]://<containername>@<accountname>.blob.<endpointSuffix>/<path>

abhishekagarwal87 · 2024-01-03T07:23:42Z

...s-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureAccountConfig.java

+    return endpointSuffix;
+  }
+
+  public String getBlobStorageEndpointSuffix()


isn't it the whole endpoint itself? why are we calling this method getBlobStorageEndpointSuffix instead of getBlobStorageEndpoint

abhishekagarwal87 · 2024-01-03T07:24:27Z

...re/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureDataSegmentPuller.java

@@ -59,7 +64,7 @@ FileUtils.FileCopyResult getSegmentFiles(
          "Loading container: [%s], with blobPath: [%s] and outDir: [%s]", containerName, blobPath, outDir
      );

-      final String actualBlobPath = AzureUtils.maybeRemoveAzurePathPrefix(blobPath);
+      final String actualBlobPath = azureUtils.maybeRemoveAzurePathPrefix(blobPath);


you could pass the endpoint suffix as an argument than passing it in the AzureUtils constructor

abhishekagarwal87

One last comment on the tests. Please do fix the conflicts as well.

abhishekagarwal87 · 2024-01-05T18:27:35Z

...ure-extensions/src/test/java/org/apache/druid/storage/azure/AzureStorageDruidModuleTest.java

+    String outputBlob = AzureUtils.maybeRemoveAzurePathPrefix("blob.core.usgovcloudapi.net/container/blob", config.getBlobStorageEndpoint());
+    Assert.assertEquals("container/blob", outputBlob);


These tests should move to AzureUtilsTest. Here you can test that AzureAccountConfig is correctly initialized with the properties you are setting. And in AzureUtilsTest, you will add these tests but without creating an AzureAccountConfig object.

vtlim · 2024-01-06T00:19:31Z

docs/development/extensions-core/azure.md

@@ -40,5 +40,6 @@ To use this Apache Druid extension, [include](../../configuration/extensions.md#
 |`druid.azure.protocol`|the protocol to use|http or https|https|
 |`druid.azure.maxTries`|Number of tries before canceling an Azure operation.| |3|
 |`druid.azure.maxListingLength`|maximum number of input files matching a given prefix to retrieve at a time| |1024|
+|`druid.azure.endpointSuffix`|The endpoint suffix to use. Could be overriden for connecting with [Azure Goverment](https://learn.microsoft.com/en-us/azure/azure-government/documentation-government-get-started-connect-to-storage#getting-started-with-storage-api).|Examples: `core.windows.net`, `core.usgovcloudapi.net`|`core.windows.net`|


Suggested change

|`druid.azure.endpointSuffix`|The endpoint suffix to use. Could be overriden for connecting with [Azure Goverment](https://learn.microsoft.com/en-us/azure/azure-government/documentation-government-get-started-connect-to-storage#getting-started-with-storage-api).|Examples: `core.windows.net`, `core.usgovcloudapi.net`|`core.windows.net`|

|`druid.azure.endpointSuffix`|The endpoint suffix to use. Override the default value to connect to [Azure Goverment](https://learn.microsoft.com/en-us/azure/azure-government/documentation-government-get-started-connect-to-storage#getting-started-with-storage-api).|Examples: `core.windows.net`, `core.usgovcloudapi.net`|`core.windows.net`|

Thanks for the suggestion, I've added your variant!

abhishekagarwal87 · 2024-01-08T13:52:22Z

...sions-core/azure-extensions/src/test/java/org/apache/druid/storage/azure/AzureUtilsTest.java

@@ -206,4 +207,18 @@ public void test_azureRetry_RunTimeExceptionWrappedInRunTimeException_returnsFal
    boolean retry = AzureUtils.AZURE_RETRY.apply(RUNTIME_EXCEPTION_WRAPPED_IN_RUNTIME_EXCEPTON);
    Assert.assertFalse(retry);
  }
+
+  @Test
+  public void testGetAzureUtilsWithDefaultProperties()


Suggested change

public void testGetAzureUtilsWithDefaultProperties()

public void testRemoveAzurePathPrefixDefaultEndpoint()

abhishekagarwal87 · 2024-01-08T13:52:39Z

...sions-core/azure-extensions/src/test/java/org/apache/druid/storage/azure/AzureUtilsTest.java

+  }
+
+  @Test
+  public void testGetAzureUtilsWithDefaultCustomBlobPath()


Suggested change

public void testGetAzureUtilsWithDefaultCustomBlobPath()

public void testRemoveAzurePathPrefixCustomEndpoint()

abhishekagarwal87 · 2024-01-08T13:53:41Z

...ure-extensions/src/test/java/org/apache/druid/storage/azure/AzureStorageDruidModuleTest.java

@@ -259,6 +259,26 @@ public void testAllCredentialsUnset()
    );
  }

+  @Test
+  public void testGetAzureUtilsWithDefaultProperties()


Suggested change

public void testGetAzureUtilsWithDefaultProperties()

public void testGetBlobStorageEndpointWithDefaultProperties()

abhishekagarwal87 · 2024-01-08T13:54:03Z

...ure-extensions/src/test/java/org/apache/druid/storage/azure/AzureStorageDruidModuleTest.java

+  }
+
+  @Test
+  public void testGetAzureUtilsWithDefaultCustomBlobPath()


Suggested change

public void testGetAzureUtilsWithDefaultCustomBlobPath()

public void testGetBlobStorageEndpointWithCustomBlobPath()

abhishekagarwal87 · 2024-01-08T17:30:06Z

docs/development/extensions-core/azure.md

-|`druid.azure.protocol`|the protocol to use|http or https|https|
-|`druid.azure.maxTries`|Number of tries before canceling an Azure operation.| |3|
-|`druid.azure.maxListingLength`|maximum number of input files matching a given prefix to retrieve at a time| |1024|
+|Property| Description                                                                                                                                                                                                                                 |Possible Values|Default|


The formatting change needs to be reverted. LGTM otherwise.

Thanks, changes reverted

abhishekagarwal87 · 2024-01-09T17:03:51Z

thank you for your contribution @nozjkoitop

sviatahorau added 2 commits December 8, 2023 11:09

Add option to configure different endpoint suffix for Azure Blob Storage

79e4b23

Azure extension docs updated

c6548f2

github-actions bot added the Area - Documentation label Dec 8, 2023

Azure extension module fixed

763542a

BartMiki reviewed Dec 12, 2023

View reviewed changes

nozjkoitop marked this pull request as ready for review December 12, 2023 09:58

abhishekagarwal87 reviewed Jan 3, 2024

View reviewed changes

sviatahorau added 2 commits January 5, 2024 14:36

Addressing code review comments

c4b94fa

Code clean up

261dfec

abhishekagarwal87 reviewed Jan 5, 2024

View reviewed changes

vtlim reviewed Jan 6, 2024

View reviewed changes

Conflicts resolved

b649629

abhishekagarwal87 reviewed Jan 8, 2024

View reviewed changes

Fixed tests and checkstyle

67d3128

abhishekagarwal87 reviewed Jan 8, 2024

View reviewed changes

sviatahorau added 2 commits January 9, 2024 10:46

docs reformat

0689d1d

fix typo

038caa9

abhishekagarwal87 merged commit ea6ba40 into apache:master Jan 9, 2024
79 checks passed

abhishekagarwal87 added the Feature label Jan 10, 2024

LakshSingla added this to the 29.0.0 milestone Jan 29, 2024

LakshSingla mentioned this pull request Feb 13, 2024

[DRAFT] 29.0.0 release notes #15896

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Azure Goverment storage #15523

Add support for Azure Goverment storage #15523

nozjkoitop commented Dec 8, 2023 •

edited

Loading

BartMiki left a comment

abhishekagarwal87 left a comment

abhishekagarwal87 Jan 3, 2024

nozjkoitop Jan 5, 2024

abhishekagarwal87 Jan 3, 2024

nozjkoitop Jan 5, 2024

abhishekagarwal87 Jan 3, 2024

nozjkoitop Jan 5, 2024

abhishekagarwal87 Jan 3, 2024

nozjkoitop Jan 5, 2024

abhishekagarwal87 Jan 3, 2024

nozjkoitop Jan 5, 2024

abhishekagarwal87 left a comment

abhishekagarwal87 Jan 5, 2024

nozjkoitop Jan 8, 2024

vtlim Jan 6, 2024

nozjkoitop Jan 8, 2024

abhishekagarwal87 Jan 8, 2024

nozjkoitop Jan 8, 2024

abhishekagarwal87 Jan 8, 2024

nozjkoitop Jan 8, 2024

abhishekagarwal87 Jan 8, 2024

nozjkoitop Jan 8, 2024

abhishekagarwal87 Jan 8, 2024

nozjkoitop Jan 8, 2024

abhishekagarwal87 Jan 8, 2024 •

edited

Loading

nozjkoitop Jan 9, 2024

abhishekagarwal87 commented Jan 9, 2024

	// wasb[s]://<containername>@<accountname>.<blobStorageEndpointSuffix>/<path>
	// wasb[s]://<containername>@<accountname>.blob.<endpointSuffix>/<path>

		String outputBlob = AzureUtils.maybeRemoveAzurePathPrefix("blob.core.usgovcloudapi.net/container/blob", config.getBlobStorageEndpoint());
		Assert.assertEquals("container/blob", outputBlob);

	\|`druid.azure.endpointSuffix`\|The endpoint suffix to use. Could be overriden for connecting with [Azure Goverment](https://learn.microsoft.com/en-us/azure/azure-government/documentation-government-get-started-connect-to-storage#getting-started-with-storage-api).\|Examples: `core.windows.net`, `core.usgovcloudapi.net`\|`core.windows.net`\|
	\|`druid.azure.endpointSuffix`\|The endpoint suffix to use. Override the default value to connect to [Azure Goverment](https://learn.microsoft.com/en-us/azure/azure-government/documentation-government-get-started-connect-to-storage#getting-started-with-storage-api).\|Examples: `core.windows.net`, `core.usgovcloudapi.net`\|`core.windows.net`\|

	public void testGetAzureUtilsWithDefaultProperties()
	public void testRemoveAzurePathPrefixDefaultEndpoint()

	public void testGetAzureUtilsWithDefaultCustomBlobPath()
	public void testRemoveAzurePathPrefixCustomEndpoint()

	public void testGetAzureUtilsWithDefaultProperties()
	public void testGetBlobStorageEndpointWithDefaultProperties()

Add support for Azure Goverment storage #15523

Add support for Azure Goverment storage #15523

Conversation

nozjkoitop commented Dec 8, 2023 • edited Loading

Description

Release note

Key changed/added classes in this PR

BartMiki left a comment

Choose a reason for hiding this comment

abhishekagarwal87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhishekagarwal87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhishekagarwal87 Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhishekagarwal87 commented Jan 9, 2024

nozjkoitop commented Dec 8, 2023 •

edited

Loading

abhishekagarwal87 Jan 8, 2024 •

edited

Loading