Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Priority Queue Cloud Pattern #122

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

skabou
Copy link
Contributor

@skabou skabou commented Mar 13, 2024

Update to .NET 8.0
Updated README

Update to .NET 8.0
Updated README
@skabou skabou requested a review from ckittel March 13, 2024 19:55
@skabou skabou force-pushed the priority-queue-update branch from c57c76c to 617a48f Compare March 13, 2024 20:24
Copy link
Member

@ckittel ckittel left a comment

Choose a reason for hiding this comment

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

Summary:

  • Use Entra ID
  • Use new Azure SDK dependency injection model
  • How do we get this to not just be a pub-sub example?
  • Use bicep for the azure resource deployments

That third item is worrying me a bit, not sure how best to proceed. Would be glad to talk it over w/ ya!

Comment on lines 42 to 45
RESOURCE_GROUP_NAME=rg-priority-queue
az group create -n $RESOURCE_GROUP_NAME -l eastus2
```
1. Provision a Service Bus messaging namespace.
Copy link
Member

Choose a reason for hiding this comment

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

For the Service Bus deployment, can you just wrap this all in a simple bicep file to do a one-step deployment? You can still use the command to get the connection string at the end.


1. Set the `PriorityQueueSender` project as startup.
1. Press F5 in Visual Studio to start running the example. The function will begin sending messages to the topic every 30 seconds.
Copy link
Member

Choose a reason for hiding this comment

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

Might be good to mention that it sends a mix of high and low priority messages.

1. Edit the `local.settings.json` file for each project set the value of `ServiceBusConnectionString` to the **SERVICE_BUS_CONNECTION_STRING** retained from earlier.

1. Set the `PriorityQueueSender` project as startup.
Copy link
Member

@ckittel ckittel Mar 14, 2024

Choose a reason for hiding this comment

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

can we just func start this? Then we wouldn't need Visual Studio installed (we're getting to the point that we either just use CLI or VSCode -- Visual Studio is "a bit much" for samples.

Comment on lines 96 to 98
1. Press F5 in Visual Studio to start the execution of the consumer function.
1. In the Azure Functions Core Tools Console, you can view the diagnostic information generated by statements in the code.
Copy link
Member

Choose a reason for hiding this comment

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

Same here, if we have the core tools installed then func start should work for us.

Comment on lines 7 to 24
.ConfigureFunctionsWorkerDefaults()
.ConfigureAppConfiguration((hostingContext, config) =>
{
config.AddJsonFile("local.settings.json", optional: true, reloadOnChange: true);
})
.ConfigureServices(services =>
{
var configuration = services.BuildServiceProvider().GetRequiredService<IConfiguration>();

services.AddSingleton(configuration);

services.AddSingleton<ServiceBusClient>(sp =>
{
var connectionString = configuration.GetValue<string>("ServiceBusConnectionString");
return new ServiceBusClient(connectionString);
});
})
.Build();
Copy link
Member

Choose a reason for hiding this comment

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

Can this get converted to:

services.AddAzureClients(cb => {
  cb.AddServiceBusClient(...)
});

https://learn.microsoft.com/azure/azure-functions/dotnet-isolated-process-guide?tabs=linux#register-azure-clients -- The PG is trying to get folks to use this model for Azure SDK clients intead of the custom approach like you have here.

Comment on lines 10 to 13
public PriorityQueueConsumerHighFn(ILogger<PriorityQueueConsumerHighFn> logger)
{
_logger = logger;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you convert to automatic constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

<None Update="local.settings.json">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
<CopyToPublishDirectory>Never</CopyToPublishDirectory>
</None>
</ItemGroup>
</Project>
<ItemGroup>
<PackageReference Include="Microsoft.Azure.Functions.Worker.Extensions.Http" Version="3.1.0" />
Copy link
Member

Choose a reason for hiding this comment

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

Is Http a requirement for something here? I thought that was only for Http triggers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Addressed/removed.

<None Update="local.settings.json">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
<CopyToPublishDirectory>Never</CopyToPublishDirectory>
</None>
</ItemGroup>
</Project>
<ItemGroup>
<PackageReference Include="Microsoft.Azure.Functions.Worker.Extensions.Http" Version="3.1.0" />
Copy link
Member

Choose a reason for hiding this comment

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

Same question on Http here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Addressed/removed.

Comment on lines 10 to 13
public PriorityQueueConsumerLowFn(ILogger<PriorityQueueConsumerLowFn> logger)
{
_logger = logger;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you convert to automatic constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

1. Set the `PriorityQueueSender` project as startup.
1. Press F5 in Visual Studio to start running the example. The function will begin sending messages to the topic every 30 seconds.
1. Stop the execution, change the startup project to either the `PriorityQueueConsumerHigh` or `PriorityQueueConsumerLow`.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not seeing how running this locally is really showing priority queues. Since both appear to be doing equal processing. There really isn't even any host.json settings to help show (e.g. maxConcurrentSessions only works for session-based access and maxMessageBatchSize only works for array input bindings). I know when deployed to Azure you can tweak instance count, but the steps here don't really seem to match what is stated at the start of the page. Maybe this sample should only have a "deploy to azure" approach so that this is actually demonstrating the pattern? Right now, it's just a pub-sub processing demo.

Copy link
Member

Choose a reason for hiding this comment

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

@skabou - what did we say next steps were about this?

Copy link
Contributor Author

@skabou skabou Apr 26, 2024

Choose a reason for hiding this comment

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

@ckittel - I have a new version in the works (deploys via bicep file) - Trying to get the PR in before the end of the month. My concern is the example is still "more or less the same" as to what is there - Essentially two pub subs, one for high priority, and one for low priority. If we don't think publishing a demo of that is worthwhile then I could scrap the code portion.

Copy link
Member

Choose a reason for hiding this comment

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

If it's an implementation of the pattern as we have it documented/described, I think it will be useful. If you don't feel though it's an implementation of the pattern, then we need to re-evaluate for sure. I think if your new version does a deploy to azure for sure, then we can show the pattern very clearly with the scale-out settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do think it's still worthwhile to demonstrate.

* Improvements. Bicep file. Settings. Moved to managed identity. Run func start

* Removing VS requirement

* deleting any

---------

Co-authored-by: Federico Arambarri <v-fearam>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants