Skip to content
Open
93 changes: 93 additions & 0 deletions Test/DurableTask.AzureStorage.Tests/TestPartitionIndex.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
// ----------------------------------------------------------------------------------
// Copyright Microsoft Corporation
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
// http://www.apache.org/licenses/LICENSE-2.0
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// ----------------------------------------------------------------------------------

namespace DurableTask.AzureStorage.Tests
{
using System;
using System.Collections.Generic;
using System.Data;
using System.Threading;
using Microsoft.VisualStudio.TestTools.UnitTesting;

[TestClass]
public class TestPartitionIndex
{
private AzureStorageOrchestrationService azureStorageOrchestrationService;
private AzureStorageOrchestrationServiceSettings settings;
private int partitionCount = 4;
private Dictionary<string, int> controlQueueNumberToNameMap;
private CancellationTokenSource cancellationTokenSource;
private const string TaskHub = "taskHubName";

[TestInitialize]
public void Initialize()
{
cancellationTokenSource = new CancellationTokenSource();

settings = new AzureStorageOrchestrationServiceSettings()
{
StorageConnectionString = TestHelpers.GetTestStorageAccountConnectionString(),
TaskHubName = TaskHub,
PartitionCount = partitionCount
};

azureStorageOrchestrationService = new AzureStorageOrchestrationService(settings);

controlQueueNumberToNameMap = new Dictionary<string, int>();

for (int i = 0; i < partitionCount; i++)
{
var controlQueueName = AzureStorageOrchestrationService.GetControlQueueName(settings.TaskHubName, i);
controlQueueNumberToNameMap[controlQueueName] = i;
}
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we still using this in the new tests? No, right?


[TestMethod]
[DataRow(20, false)]
[DataRow(20, true)]
public void GetPartitionIndexTest(int maxInstanceIdCount, bool enableExplicitPartitionPlacement)
{
settings.EnableExplicitPartitionPlacement = enableExplicitPartitionPlacement;

for (uint instanceIdSuffix = 0; instanceIdSuffix < settings.PartitionCount * 4; instanceIdSuffix++)
{
Dictionary<uint, int> indexNumberToCount = new Dictionary<uint, int>();

for (uint indexCount = 0; indexCount < settings.PartitionCount; indexCount++)
{
indexNumberToCount[indexCount] = 0;
}

for (int instanceCount = 0; instanceCount < maxInstanceIdCount; instanceCount++)
{
var instanceIdPrefix = Guid.NewGuid().ToString();

var instanceId = $"{instanceIdPrefix}!{instanceIdSuffix}";

var partitionIndex = azureStorageOrchestrationService.GetPartitionIndex(instanceId);

indexNumberToCount[partitionIndex]++;
}

if (enableExplicitPartitionPlacement)
{
Assert.AreEqual(indexNumberToCount[(uint)(instanceIdSuffix % settings.PartitionCount)], maxInstanceIdCount);
}
else
{
Assert.AreNotEqual(indexNumberToCount[(uint)(instanceIdSuffix % settings.PartitionCount)], maxInstanceIdCount);
}
}
}
}
}
26 changes: 24 additions & 2 deletions src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2048,9 +2048,9 @@ public Task<string> DownloadBlobAsync(string blobUri)

// TODO: Change this to a sticky assignment so that partition count changes can
// be supported: https://github.com/Azure/azure-functions-durable-extension/issues/1
async Task<ControlQueue?> GetControlQueueAsync(string instanceId)
internal async Task<ControlQueue?> GetControlQueueAsync(string instanceId)
{
uint partitionIndex = Fnv1aHashHelper.ComputeHash(instanceId) % (uint)this.settings.PartitionCount;
uint partitionIndex = GetPartitionIndex(instanceId);
string queueName = GetControlQueueName(this.settings.TaskHubName, (int)partitionIndex);

ControlQueue cachedQueue;
Expand All @@ -2075,6 +2075,28 @@ public Task<string> DownloadBlobAsync(string blobUri)
return cachedQueue;
}

internal uint GetPartitionIndex(string instanceId)
{
uint totalPartitions = (uint)this.settings.PartitionCount;

int placementSeparatorPosition = instanceId.LastIndexOf('!');

// if the instance id ends with !nnn, where nnn is an unsigned number, it indicates explicit partition placement
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a test that documents the behavior if the customer uses an instanceID with multiple ! in there? Say instanceID "A!1!B!3` should probably map to partition "3", right?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also add a test that checks that instanceID myinstanceID!NotANumber does not trigger any errors / that it correctly ignores the explicit placement logic.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding the first test, I think we're just missing the very last one:

Let's also add a test that checks that instanceID myinstanceID!NotANumber does not trigger any errors / that it correctly ignores the explicit placement logic.

if (
this.settings.EnableExplicitPartitionPlacement
&& placementSeparatorPosition != -1
&& uint.TryParse(instanceId.Substring(placementSeparatorPosition + 1), out uint index))
{
var partitionId = index % totalPartitions;
return (uint)partitionId;
Comment thread
davidmrdavid marked this conversation as resolved.
}
else
{
return Fnv1aHashHelper.ComputeHash(instanceId) % totalPartitions;

}
}

/// <summary>
/// Disposes of the current object.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -318,5 +318,11 @@ internal LogHelper Logger
/// Consumers that require separate dispatch (such as the new out-of-proc v2 SDKs) must set this to true.
/// </summary>
public bool UseSeparateQueueForEntityWorkItems { get; set; } = false;

/// <summary>
/// Enabled explicit placement of instance to parition id.
/// if the instance id ends with !nnn, where nnn is an unsigned number, it indicates explicit partition placement
/// </summary>
Comment thread
pasaini-microsoft marked this conversation as resolved.
Outdated
Comment on lines +322 to +325
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - I believe remark is the right docs tag for this sort of thing. Let's also not use all-caps for comments, I realize it has a useful effect, but it's not in our usual style for customer-facing docs.

Suggested change
/// <summary>
/// Whether to allow instanceIDs to use special syntax to land on a specific partition.
/// If enabled, when an instanceID ends with suffix '!nnn', where 'nnn' is an unsigned number, the instance will land on the partition/queue for to that number.
/// ** DO NOT CHANGE THIS FLAG FOR PRE-EXISTING MESSAGES AS IT MAY BE CONSIDERED IN THE WRONG QUEUE **
/// </summary>
/// <summary>
/// Whether to allow instanceIDs to use special syntax to land on a specific partition.
/// If enabled, when an instanceID ends with suffix '!nnn', where 'nnn' is an unsigned number, the instance will land on the partition/queue for to that number.
/// ** DO NOT CHANGE THIS FLAG FOR PRE-EXISTING MESSAGES AS IT MAY BE CONSIDERED IN THE WRONG QUEUE **
/// </summary>
/// <remarks>
/// It is not generally safe to change to this flag for pre-existing TaskHubs, as it may change the expected target queue for an instanceID.
/// </remarks>

public bool EnableExplicitPartitionPlacement { get; set; } = false;
Comment on lines +322 to +329
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something to consider - it is not safe to change this from false to true (or vice-versa) while an orchestrator with the special syntax is in-flight. If we do that, any pre-existing messages for that orchestrator may now be considered to be "in the wrong queue".

Let's call this out in the intellisense

}
}