Switch spec from docker to locally run azurite instance #207

Arkatufus · 2022-04-18T17:58:20Z

Uses locally installed azurite using npm
Upgrade Windows.Azure.Cosmos.Table package to Azure.Data.Tables

NOTE:
~~Will fail until akkadotnet/akka.net#5849 is merged and released~~

build-system/azure-pipeline.template.yaml

src/Akka.Persistence.Azure/Journal/AzureTableStorageJournal.cs

Arkatufus · 2022-05-24T14:34:21Z

src/Akka.Persistence.Azure/Journal/AzureTableStorageJournal.cs


-                            batchItems = batchItems.Add(newItem);
+                            batchItems.Add(new TableTransactionAction(TableTransactionActionType.Add, newItem.WriteEntity()));


New SDK way of building a batch data CRUD, everything is encapsulated inside a transaction-like batch

Arkatufus · 2022-05-24T14:37:51Z

src/Akka.Persistence.Azure/Journal/AzureTableStorageJournal.cs

+                filter: $"PartitionKey eq '{AllPersistenceIdsEntry.PartitionKeyValue}'",
+                maxPerPage: maxPerPage,
+                @select: new[] { "RowKey" }, 
+                cancellationToken: cancellationToken


The biggest change in the SDK, it doesn't use an awkward expression builder anymore, it actually takes an OData query directly. We could also use a strongly typed Expression<Func<bool>> linq-like builder, but that turns out to be problematic for building string comparison queries.

A little cheat-sheet on OData:

eq is equals

ne is not equals

lt is less than

le is less than or equal

gt is greater than

ge is greater than or equal

Arkatufus · 2022-05-24T14:39:32Z

src/Akka.Persistence.Azure/Journal/AzureTableStorageJournal.cs

+                        $"RowKey le '{toSequenceNr.ToJournalRowKey()}'",
+                maxPerPage: maxPerPage,
+                @select: null,
+                cancellationToken: cancellationToken);


Comparing this and the old way of building queries, you can see that this is a lot more clear and concise.

src/Akka.Persistence.Azure/Journal/AzureTableStorageJournal.cs

Arkatufus · 2022-05-24T14:45:56Z

src/Akka.Persistence.Azure/Journal/AzureTableStorageJournal.cs

            }
        }

+        private async Task<bool> IsTableExist(string name, CancellationToken token)


The Table.Exists method was removed by design in the new SDK, so we need to roll our own here.

Arkatufus · 2022-05-24T14:49:15Z

src/Akka.Persistence.Azure/Journal/HighestSequenceNrEntry.cs

-
-namespace Akka.Persistence.Azure.Journal
-{
-    internal sealed class HighestSequenceNrEntry


This class is a duplicate of Akka.Persistence.Azure.TableEntities.HighestSequenceNrEntry, deleting it so that it doesn't cause any bug in the future.

Arkatufus · 2022-05-24T14:50:59Z

src/Akka.Persistence.Azure/TableEntities/AllPersistenceIdsEntry.cs

    {
        private const string ManifestKeyName = "manifest";
        public const string PartitionKeyValue = "allPersistenceIdsIdx";

        // In order to use this in a TableQuery a parameterless constructor is required
-        public AllPersistenceIdsEntry()
+        public AllPersistenceIdsEntry(TableEntity entity)


Instead of relying on the SDK deserialization code, we will use the EventHubs generic TableEntity serialization and populate our own data from it.
By doing this, we can convert all of the table entity data model back to immutable classes.

I think you mean Table Storage generic serialization, but your point is well taken.

src/Akka.Persistence.Azure/TableEntities/AllPersistenceIdsEntry.cs

Arkatufus · 2022-05-24T15:00:34Z

src/Akka.Persistence.Azure/TableEntities/PersistentJournalEntry.cs

-        public string[] Tags { get; set; }
-
-        public DateTimeOffset Timestamp { get; set; }
+        public string[] Tags { get; }


This property is the main reason why I opted not to use the built-in serializer. In the strongly typed json based serializer, this would be saved as a JSON array, while we traditionally have been saving it as a CSV. Trying to coerce the internal JSON serializer to serialize this as CSV would be a mess.

Aaronontheweb

Not quite done reviewing yet, but left some thoughts / questions.

Very interested in seeing if we can get #129 (comment) resolved as well.

build-system/azure-pipeline.template.yaml

Aaronontheweb · 2022-05-25T03:54:38Z

src/Akka.Persistence.Azure.Tests/AzureTableJournalEscapePersistentIdSpec.cs

+            Initialize();
+        }
+
+        [WindowsFact(SkipUnixReason = "Batch delete is not supported by Azurite in Linux")]


Aaronontheweb · 2022-05-25T03:57:28Z

src/Akka.Persistence.Azure.Tests/Helper/AzureStorageConfigHelper.cs

+
+        public static Config AzureConfig(Func<string, Config> configTemplate)
+        {
+            var connString = Environment.GetEnvironmentVariable("AZURE_CONNECTION_STR");


src/Akka.Persistence.Azure/Journal/AzureTableStorageJournal.cs

Aaronontheweb · 2022-05-25T04:38:41Z

src/Akka.Persistence.Azure/Journal/AzureTableStorageJournal.cs

-                                foreach (var r in eventTagsResults)
-                                    _log.Debug("Azure table storage wrote entity [{0}] with status code [{1}]", r.Etag, r.HttpStatusCode);
+                                foreach (var r in eventTagsResponse.Value)
+                                    _log.Debug("Azure table storage wrote entity with status code [{1}]", r.Status);

                            if (HasTagSubscribers && taggedEntries.Count != 0)


Should we get rid of the tag subscribers stuff inside the write loop? Seems out of place given that we still have to poll for changes on the table to detect changes that happen exclusively on other nodes in the network. This is something that got copied and pasted into a lot of journals but it was only ever really appropriate for SQLite / in-memory.

We should, should we open another issue and PR for this?

Yes we should

Aaronontheweb · 2022-05-25T04:38:54Z

src/Akka.Persistence.Azure/Journal/AzureTableStorageJournal.cs

@@ -453,7 +411,7 @@ protected override void PreStart()
                 *
                 * Either everything fails or everything succeeds is the idea I guess.
                 */
-                return exceptions.Any(ex => ex != null) ? exceptions : null;
+                return exceptions.Any(ex => ex != null) ? exceptions.ToImmutableList() : null;


Arkatufus · 2022-05-25T17:26:03Z

To compute the highest sequence number, is that a constant time operation (peeking the top of the partition) or do we have to scan a portion of it? Been a while since I've looked at this code.

The query will grab all persistence id entries filtered to their last row key, then the code will compare all of those entries for the highest sequence number. In theory, this will be slow for journal tables with a lot of persistence ids.

Arkatufus · 2022-05-25T17:27:24Z

That's just to shave off some latency?

Yes, the hack shaves some query over HTTP latency for the next page in the query.

Aaronontheweb

LGTM

Aaronontheweb · 2022-05-27T14:17:45Z

src/Akka.Persistence.Azure/Journal/AzureTableStorageJournal.cs

-            var returnValue = new TableQuery<HighestSequenceNrEntry>().Where(filter);
-
-            return returnValue;
+            return Table.QueryAsync<TableEntity>(


This is a constant time query since it looks up a specific table entry for each entity that stores the highestSequenceNr

Aaronontheweb · 2022-05-27T14:19:00Z

src/Akka.Persistence.Azure/Journal/AzureTableStorageJournal.cs

+            get
+            {
+                if (_tableStorage_DoNotUseDirectly == null)
+                    throw new Exception("Table storage has not been initialized yet. PreStart() has not been invoked");


src/Akka.Persistence.Azure/Journal/AzureTableStorageJournal.cs

Aaronontheweb · 2022-05-27T14:22:48Z

src/Akka.Persistence.Azure/Journal/AzureTableStorageJournal.cs

-                                foreach (var r in eventTagsResults)
-                                    _log.Debug("Azure table storage wrote entity [{0}] with status code [{1}]", r.Etag, r.HttpStatusCode);
+                                foreach (var r in eventTagsResponse.Value)
+                                    _log.Debug("Azure table storage wrote entity with status code [{1}]", r.Status);

                            if (HasTagSubscribers && taggedEntries.Count != 0)


Yes we should

Aaronontheweb · 2022-05-27T14:23:28Z

src/Akka.Persistence.Azure/Journal/AzureTableStorageJournal.cs

-                        var encodedKey = PartitionKeyEscapeHelper.Escape(x.Key);
-                        allPersistenceIdsBatch.InsertOrReplace(new AllPersistenceIdsEntry(encodedKey));
-                    });
+                        allPersistenceIdsBatch.Add(new TableTransactionAction(


Should add a comment here explaining that this is updating a specific object that stores the highest seqNo used in recovery

Aaronontheweb · 2022-05-27T14:27:21Z

src/Akka.Persistence.Azure/TableEntities/AllPersistenceIdsEntry.cs

    {
        private const string ManifestKeyName = "manifest";
        public const string PartitionKeyValue = "allPersistenceIdsIdx";

        // In order to use this in a TableQuery a parameterless constructor is required
-        public AllPersistenceIdsEntry()
+        public AllPersistenceIdsEntry(TableEntity entity)


I think you mean Table Storage generic serialization, but your point is well taken.

src/Akka.Persistence.Azure/TableEntities/AllPersistenceIdsEntry.cs

Aaronontheweb · 2022-05-27T14:30:32Z

src/Akka.Persistence.Azure/TableEntities/PersistentJournalEntry.cs

+                Timestamp = Timestamp,
+                [PayloadKeyName] = Payload,
+                [SeqNoKeyName] = SeqNo,
+                [TagsKeyName] = string.Join(";", Tags),


Looks the same as before, good job

Arkatufus added 8 commits April 19, 2022 00:57

Switch spec from docker to locally run azurite instance

d9d949e

Merge branch 'dev' into Switch_spec_to_Azurite

c5ffa6c

Fix build script

90c59de

Fix build script

7c637de

Bump Akka.NET to 1.4.37

25f753e

Add WindowsFactAttribute

9ebd45a

Merge branch 'dev' into Switch_spec_to_Azurite

4ea789d

Update Microsoft.Azure.Cosmos.Table to Azure.Data.Tables

a66705a

Arkatufus requested a review from Aaronontheweb May 20, 2022 03:16

Arkatufus self-assigned this May 20, 2022

Arkatufus added the dependencies Pull requests that update a dependency file label May 20, 2022

Merge branch 'dev' into Switch_spec_to_Azurite

6804cc2

Arkatufus commented May 24, 2022

View reviewed changes

build-system/azure-pipeline.template.yaml Show resolved Hide resolved

Arkatufus commented May 24, 2022

View reviewed changes

build-system/azure-pipeline.template.yaml Show resolved Hide resolved

Arkatufus commented May 24, 2022

View reviewed changes

src/Akka.Persistence.Azure/Journal/AzureTableStorageJournal.cs Show resolved Hide resolved

Arkatufus commented May 24, 2022

View reviewed changes

src/Akka.Persistence.Azure/Journal/AzureTableStorageJournal.cs Show resolved Hide resolved

Arkatufus commented May 24, 2022

View reviewed changes

src/Akka.Persistence.Azure/Journal/AzureTableStorageJournal.cs Show resolved Hide resolved

Arkatufus commented May 24, 2022

View reviewed changes

src/Akka.Persistence.Azure/Journal/AzureTableStorageJournal.cs Show resolved Hide resolved

Arkatufus commented May 24, 2022

View reviewed changes

src/Akka.Persistence.Azure/Journal/AzureTableStorageJournal.cs Show resolved Hide resolved

Arkatufus commented May 24, 2022

View reviewed changes

src/Akka.Persistence.Azure/TableEntities/AllPersistenceIdsEntry.cs Show resolved Hide resolved

Arkatufus commented May 24, 2022

View reviewed changes

Aaronontheweb linked an issue May 25, 2022 that may be closed by this pull request

Migrate test suite to Azurite instead of old azure emulator image #106

Closed

Aaronontheweb reviewed May 25, 2022

View reviewed changes

Aaronontheweb approved these changes May 27, 2022

View reviewed changes

Aaronontheweb merged commit 9bf6f47 into petabridge:dev May 27, 2022

This was referenced May 27, 2022

[Snapshot] Cosmos SQL API #125

Closed

Remove tag subscriber updates from inner write loop #216

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch spec from docker to locally run azurite instance #207

Switch spec from docker to locally run azurite instance #207

Arkatufus commented Apr 18, 2022 •

edited

Loading

Arkatufus May 24, 2022

Arkatufus May 24, 2022

Arkatufus May 24, 2022

Arkatufus May 24, 2022

Arkatufus May 24, 2022

Arkatufus May 24, 2022

Arkatufus May 24, 2022 •

edited

Loading

Aaronontheweb May 27, 2022

Arkatufus May 24, 2022

Aaronontheweb left a comment

Aaronontheweb May 25, 2022

Aaronontheweb May 25, 2022

Aaronontheweb May 25, 2022

Arkatufus May 25, 2022

Aaronontheweb May 27, 2022

Aaronontheweb May 25, 2022

Arkatufus commented May 25, 2022

Arkatufus commented May 25, 2022

Aaronontheweb left a comment

Aaronontheweb May 27, 2022

Aaronontheweb May 27, 2022

Aaronontheweb May 27, 2022

Aaronontheweb May 27, 2022

Aaronontheweb May 27, 2022

Aaronontheweb May 27, 2022


		batchItems = batchItems.Add(newItem);
		batchItems.Add(new TableTransactionAction(TableTransactionActionType.Add, newItem.WriteEntity()));

Switch spec from docker to locally run azurite instance #207

Switch spec from docker to locally run azurite instance #207

Conversation

Arkatufus commented Apr 18, 2022 • edited Loading

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

Arkatufus May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aaronontheweb 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

Arkatufus commented May 25, 2022

Arkatufus commented May 25, 2022

Aaronontheweb 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

Arkatufus commented Apr 18, 2022 •

edited

Loading

Arkatufus May 24, 2022 •

edited

Loading