RavenDB-21956 Add azure queue storage etl #18293

djordjedjukic · 2024-03-26T13:06:29Z

Issue link

https://issues.hibernatingrhinos.com/issue/RavenDB-21956/ETL-to-Azure-Queue-Storage

Additional description

Expanding Queue ETL with Azure Queue Storage provider.

Type of change

New feature

How risky is the change?

Not relevant

Backward compatibility

Non breaking change

Is it platform specific issue?

No

Documentation update

This change requires a documentation update. Please mark the issue on YouTrack using Documentation Required tag.

Testing by Contributor

Tests have been added that prove the fix is effective or that the feature works

Testing by RavenDB QA team

This change requires a special QA testing due to possible performance or resources usage implications (CPU, memory, IO). Please mark the issue on YouTrack using QA Required tag.

Is there any existing behavior change of other features due to this change?

No

UI work

It requires further work in the Studio. Please mark the issue on YouTrack using Studio Required tag.

arekpalinski · 2024-03-28T13:13:25Z

src/Raven.Client/Documents/Operations/ETL/Queue/AzureQueueStorageConnectionSettings.cs

+{
+    public EntraId EntraId { get; set; }
+    public string ConnectionString { get; set; }
+    public bool Passwordless { get; set; }


Why those aren't directly part of AzureQueueStorageConnectionSettings? Do we need AzureQueueStorageConnectionSettings.Authentication field?

done
(I followed how we did for Elasticsearch, but maybe there also should be directly part of connection settings).

In case of ElasticSearchConnectionString it was a bit different because we provide node's URLs separately and then the authentication. Here we don't have such separation

arekpalinski · 2024-03-28T13:17:04Z

src/Raven.Client/Documents/Operations/ETL/Queue/QueueConnectionString.cs

@@ -31,6 +33,12 @@ protected override void ValidateImpl(ref List<string> errors)
                    errors.Add($"{nameof(RabbitMqConnectionSettings)} has no valid setting.");
                }
                break;
+            case QueueBrokerType.AzureQueueStorage:
+                if (AzureQueueStorageConnectionSettings?.Authentication == null)


Just checking if it's not null is enough? I think we should rather verify that what was provided there is correct. For example that there is just one auth option provided and that is has all fields / properties filled correctly

src/Raven.Client/Documents/Operations/ETL/Queue/AzureQueueStorageConnectionSettings.cs

src/Raven.Client/Documents/Operations/ETL/Queue/QueueEtlConfiguration.cs

arekpalinski · 2024-03-28T13:22:30Z

src/Raven.Server/Commercial/LicenseStatus.cs

@@ -216,7 +216,9 @@ public bool CanAutoRenewLetsEncryptCertificate

        public bool HasElasticSearchEtl => Enabled(LicenseAttribute.ElasticSearchEtl);

-        public bool HasQueueEtl => Enabled(LicenseAttribute.QueueEtl);
+        //public bool HasQueueEtl => Enabled(LicenseAttribute.QueueEtl);
+        public bool HasQueueEtl => true;


Please make sure it will be reverted

arekpalinski · 2024-03-28T13:41:55Z

...ders/Queue/Handlers/Processors/QueueEtlHandlerProcessorForTestAzureQueueStorageConnection.cs

+        try
+        {
+            string authenticationJson = await new StreamReader(HttpContext.Request.Body).ReadToEndAsync();
+            Authentication authentication = JsonConvert.DeserializeObject<Authentication>(authenticationJson);


Why just Authentication instead of entire connection string?

arekpalinski · 2024-03-28T13:44:02Z

src/Raven.Server/Documents/ETL/Providers/Queue/QueueBrokerConnectionHelper.cs

+        }
+        else
+        {
+            throw new NotSupportedException("Provided authentication method is not supported");


Please make sure it will be validate much earlier - on attempt to put a connection string. We should never get here

Just removed this part. You can not create auth option that doesn't exist in AzureQueueStorageConnectionSettings

arekpalinski · 2024-03-28T13:44:53Z

src/Raven.Server/Raven.Server.csproj

@@ -155,7 +155,12 @@
    <PackageReference Include="AWSSDK.Core" Version="3.7.302.16" />
    <PackageReference Include="AWSSDK.Glacier" Version="3.7.300.55" />
    <PackageReference Include="AWSSDK.S3" Version="3.7.305.31" />
+    <PackageReference Include="AWSSDK.Core" Version="3.7.302.9" />
+    <PackageReference Include="AWSSDK.Glacier" Version="3.7.300.49" />
+    <PackageReference Include="AWSSDK.S3" Version="3.7.305.25" />


It seems like a merge conflict - see duplicated AWSSDK references

arekpalinski · 2024-03-28T13:47:25Z

test/SlowTests/Server/Documents/ETL/Queue/AzureQueueStorageEtlTests.cs

+
+namespace SlowTests.Server.Documents.ETL.Queue;
+
+public class AzureQueueStorageEtlTests : AzureQueueStorageEtlTestBase


Don't we need to do any cleanup of queues at the end of each test?

I achieved it in this way:
https://github.com/djordjedjukic/ravendb/blob/051ba2272c0ec20d347b3bfd07d9a518a432a845/test/SlowTests/Server/Documents/ETL/Queue/AzureQueueStorageEtlTestBase.cs#L91-L96

Should I change it?

I was rather thinking about deleting queues during the dispose of tests

When you shut down azurite everything is deleted.

Do you think we need to delete queues explicitly?

In our case we'll rather have Azurite emulator always running (in docker) so the cleanup after tests is necessary

arekpalinski · 2024-03-28T13:48:58Z

test/SlowTests/Server/Documents/ETL/Queue/AzureQueueStorageEtlTests.cs

+
+            Assert.Empty(record.QueueEtls);
+        }
+    }


Can we have tests which intentionally try to push too big messages?

poissoncorp · 2024-04-03T10:37:37Z

test/SlowTests/Server/Documents/ETL/Queue/AzureQueueStorageEtlTests.cs

+    }
+
+    [RavenFact(RavenTestCategory.BackupExportImport | RavenTestCategory.Sharding | RavenTestCategory.Etl)]
+    public async Task ShouldSkipUnsupportedFeaturesInShardingOnImport_RabbitMqEtl()


Suggested change

public async Task ShouldSkipUnsupportedFeaturesInShardingOnImport_RabbitMqEtl()

public async Task ShouldSkipUnsupportedFeaturesInShardingOnImport_AzureQueueStorage()

src/Raven.Client/Documents/Operations/ETL/Queue/AzureQueueStorageConnectionSettings.cs

src/Raven.Client/Documents/Operations/ETL/Queue/QueueBrokerType.cs

arekpalinski · 2024-04-17T09:44:28Z

src/Raven.Server/Config/Categories/EtlConfiguration.cs

+        public TimeSetting AzureQueueStorageTimeToLive{ get; set; }
+
+        [Description("How long a message is hidden after being retrieved but not deleted")]
+        [DefaultValue(0)]


It's also the default value in Azure right?

src/Raven.Server/Dashboard/DatabasesOngoingTasksInfo.cs

src/Raven.Server/Documents/ETL/EtlProcess.cs

arekpalinski · 2024-05-06T10:14:44Z

Please do the rebase and solve the conflicts

arekpalinski · 2024-05-08T13:53:44Z

There is a studio compilation issue here. @kalczur or @ml054 can you please check it?

djordjedjukic · 2024-05-08T13:55:19Z

There is a studio compilation issue here. @kalczur or @ml054 can you please check it?

Probably I made a mistake during resolving conflicts.

ml054 · 2024-05-08T21:03:17Z

@djordjedjukic Was there any c# conflicts? If no maybe @kalczur can rebase again?

@kalczur It looks like conflict with pinning you were adding last time. I assume we need to apply same changes in this PR.

kalczur · 2024-05-09T06:34:33Z

@djordjedjukic Was there any c# conflicts? If no maybe @kalczur can rebase again?

@kalczur It looks like conflict with pinning you were adding last time. I assume we need to apply same changes in this PR.

sure, I'll fix it

RavenDB-21956 Allow to pin responsible node in azure queue storage etl

arekpalinski · 2024-05-15T11:10:07Z

src/Raven.Server/Raven.Server.csproj

+    <PackageReference Include="AWSSDK.Core" Version="3.7.302.16" />
+    <PackageReference Include="AWSSDK.Glacier" Version="3.7.300.55" />
+    <PackageReference Include="AWSSDK.S3" Version="3.7.305.31" />    
+    <PackageReference Include="Azure.Identity" Version="1.10.4" />


Please remove that

I meant AWSSDK.* - because it's duplicated

arekpalinski · 2024-05-15T13:13:03Z

src/Raven.Client/Documents/Operations/ETL/Queue/AzureQueueStorageConnectionSettings.cs

+    {
+        var parts = connectionString.Split(';')
+            .Select(p => p.Split('='))
+            .ToDictionary(p => p[0], p => p.Length > 1 ? p[1] : "");


I think you could use SqlConnectionStringParser.GetConnectionStringValue() here

arekpalinski · 2024-05-15T13:15:16Z

src/Raven.Client/Documents/Operations/ETL/Queue/AzureQueueStorageConnectionSettings.cs

+        return storageAccountName;
+    }
+
+    private void HandleConnectionStringError(string message)


"Handle" doesn't suggest this method will throw. Rename do ThrowConnectionStringError() maybe?

arekpalinski · 2024-05-15T13:16:50Z

src/Raven.Client/Documents/Operations/ETL/Queue/AzureQueueStorageConnectionSettings.cs

+        return $"https://{storageAccountName}.queue.core.windows.net/";
+    }
+
+    private string GetUrlFromConnectionString(string connectionString)


Can we have unit tests for method? So we'll know that we properly deal with connection strings (it can be handled with the usage of a public GetStorageUrl() method)

arekpalinski · 2024-05-15T13:20:42Z

...rver/Documents/ETL/Providers/Queue/AzureQueueStorage/AzureQueueStorageDocumentTransformer.cs

+            args.Length == 2 ? args[1].AsObject() : null);
+    }
+
+    private JsValue LoadToFunctionTranslatorWithAttributesInternal(string name, ObjectInstance obj,


Can we do something about that?

arekpalinski · 2024-05-15T13:21:59Z

src/Raven.Server/Documents/ETL/Providers/Queue/AzureQueueStorage/AzureQueueStorageEtl.cs

+                CreateQueueIfNotExists(queueClient);
+
+            foreach (AzureQueueStorageItem queueItem in queue.Items)
+            {


I think we should have CancellationToken.ThrowIfCancellationRequested(); here in order to stop processing if the cancellation was requested

poissoncorp · 2024-05-15T13:56:53Z

src/Raven.Client/Documents/Operations/ETL/Queue/AzureQueueStorageConnectionSettings.cs

+
+    public bool IsValid()
+    {
+        return !string.IsNullOrWhiteSpace(StorageAccountName) &&


Suggested change

return !string.IsNullOrWhiteSpace(StorageAccountName) &&

return string.IsNullOrWhiteSpace(StorageAccountName) == false &&

improves readability, same goes for lines below

poissoncorp · 2024-05-15T13:58:27Z

src/Raven.Client/Documents/Operations/ETL/Queue/AzureQueueStorageConnectionSettings.cs

+
+    public bool IsValid()
+    {
+        return !string.IsNullOrWhiteSpace(StorageAccountName);


Suggested change

return !string.IsNullOrWhiteSpace(StorageAccountName);

return string.IsNullOrWhiteSpace(StorageAccountName) == false;

poissoncorp

newest Arek's comments mostly reflect also my thoughts

github-actions bot added the v6.0 label Mar 26, 2024

arekpalinski requested a review from poissoncorp March 28, 2024 13:09

arekpalinski added the Future label Mar 28, 2024

arekpalinski reviewed Mar 28, 2024

View reviewed changes

poissoncorp reviewed Apr 3, 2024

View reviewed changes

djordjedjukic force-pushed the RavenDB-21956 branch 2 times, most recently from b376de7 to 38c7e30 Compare April 10, 2024 11:32

kalczur requested a review from ml054 April 12, 2024 10:59

kalczur force-pushed the RavenDB-21956 branch from b321213 to f90ee1e Compare April 12, 2024 11:12

ml054 approved these changes Apr 14, 2024

View reviewed changes

arekpalinski reviewed Apr 17, 2024

View reviewed changes

djordjedjukic force-pushed the RavenDB-21956 branch from 24c3a93 to 6143c8b Compare May 8, 2024 14:09

djordjedjukic force-pushed the RavenDB-21956 branch from 0726454 to 88c7708 Compare May 12, 2024 11:35

RavenDB-21956 Add Azure Queue Storage ETL

dd04b8b

RavenDB-21956 Allow to pin responsible node in azure queue storage etl

djordjedjukic force-pushed the RavenDB-21956 branch from 88c7708 to dd04b8b Compare May 12, 2024 11:36

djordjedjukic marked this pull request as ready for review May 12, 2024 19:10

arekpalinski requested review from poissoncorp and arekpalinski May 13, 2024 05:31

arekpalinski reviewed May 15, 2024

View reviewed changes

poissoncorp reviewed May 15, 2024

View reviewed changes

poissoncorp reviewed May 16, 2024

View reviewed changes


		namespace SlowTests.Server.Documents.ETL.Queue;

		public class AzureQueueStorageEtlTests : AzureQueueStorageEtlTestBase

	public async Task ShouldSkipUnsupportedFeaturesInShardingOnImport_RabbitMqEtl()
	public async Task ShouldSkipUnsupportedFeaturesInShardingOnImport_AzureQueueStorage()

	return !string.IsNullOrWhiteSpace(StorageAccountName) &&
	return string.IsNullOrWhiteSpace(StorageAccountName) == false &&

RavenDB-21956 Add azure queue storage etl #18293

Are you sure you want to change the base?

RavenDB-21956 Add azure queue storage etl #18293

Conversation

djordjedjukic commented Mar 26, 2024 • edited by ayende

Issue link

Additional description

Type of change

How risky is the change?

Backward compatibility

Is it platform specific issue?

Documentation update

Testing by Contributor

Testing by RavenDB QA team

Is there any existing behavior change of other features due to this change?

UI work

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

djordjedjukic Apr 9, 2024 • edited

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

djordjedjukic Apr 9, 2024 • edited

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

poissoncorp Apr 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arekpalinski commented May 6, 2024

arekpalinski commented May 8, 2024

djordjedjukic commented May 8, 2024

ml054 commented May 8, 2024

kalczur commented May 9, 2024

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

poissoncorp left a comment

Choose a reason for hiding this comment

djordjedjukic commented Mar 26, 2024 •

edited by ayende

djordjedjukic Apr 9, 2024 •

edited

djordjedjukic Apr 9, 2024 •

edited

poissoncorp Apr 3, 2024 •

edited