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

Add Service Bus Sessions Example #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rsr-maersk
Copy link

No description provided.

Copy link
Member

@phatboyg phatboyg left a comment

Choose a reason for hiding this comment

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

Interesting effort, but a lot of confusing/unnecessary settings here that will just confuse people.

@@ -29,6 +31,10 @@ public static IHostBuilder CreateHostBuilder(string[] args)

x.AddConsumer<SubmitOrderConsumer>();
x.AddConsumer<OrderSubmittedConsumer>();
x.AddConsumer<OrderShippedConsumer>(e =>
{
e.UseTimeout(c => c.Timeout = TimeSpan.FromSeconds(10));
Copy link
Member

Choose a reason for hiding this comment

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

This isn't really used and probably just confuses people.

Copy link
Author

Choose a reason for hiding this comment

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

Won't it fail the SB and not ack of the message handling takes to long?
Ie it will ensure failed process are put to failed?

But you are right it can be found easy enough, not needed here

e.MaxDeliveryCount = 2;
e.RequiresSession = true;
e.MaxConcurrentCalls = 8; //like AZ Function default. 8 Concurrent consumers handling 1 ServiceBus Session
e.PrefetchCount = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Not really a valid value you should specify.

Copy link
Author

Choose a reason for hiding this comment

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

I guess you are right, it isn't needed AZ funcs prefect too with peek, as long as the prefect does not ack and respects sessions.
I believe it does, yes?

e.AutoDeleteOnIdle = TimeSpan.FromDays(30);
e.MaxDeliveryCount = 2;
e.RequiresSession = true;
e.MaxConcurrentCalls = 8; //like AZ Function default. 8 Concurrent consumers handling 1 ServiceBus Session
Copy link
Member

Choose a reason for hiding this comment

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

Actually, no, MaxConcurrentCallsPerSession is the number of concurrent calls per session, which defaults to unassigned.

ConcurrentMessageLimit ultimately maps down to MaxConcurrentCalls at the transport level. I would avoid using the SB specific property here, but it's a subscription endpoint, so whatever.

Copy link
Author

@rsr-maersk rsr-maersk Sep 5, 2023

Choose a reason for hiding this comment

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

ConcurrentMessageLimit didn't respect sessions.
We had same session id coming on 2 threads.

I have a test / sample proving this. I will add here tomorrow

cfg.SubscriptionEndpoint<OrderShipped>("order-shipped-consumer", e =>
{
e.ConfigureConsumeTopology = false;
e.AutoStart = true;
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary, noise.

Copy link
Author

Choose a reason for hiding this comment

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

True

cfg.Publish<OrderShippedBase>(configurator => configurator.Exclude = true);
cfg.SubscriptionEndpoint<OrderShipped>("order-shipped-consumer", e =>
{
e.ConfigureConsumeTopology = false;
Copy link
Member

Choose a reason for hiding this comment

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

Also unnecessary noise.

Copy link
Author

@rsr-maersk rsr-maersk Sep 5, 2023

Choose a reason for hiding this comment

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

Is needed otherwise it will make unwanted service bus topics for interfaces and inheritance

You have a discussion recommending this?

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 referring to the ConfigureConsumeTopology = false

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.

2 participants