⚠ This page is served via a proxy. Original site: https://github.com
This service does not collect credentials or authentication data.
Skip to content

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.

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