Skip to content

Fix producer max pending messages validation inconsistency#284

Merged
blankensteiner merged 3 commits intoapache:masterfrom
AmiradelBeyg:fix-maxpendingmessages-default
Mar 19, 2026
Merged

Fix producer max pending messages validation inconsistency#284
blankensteiner merged 3 commits intoapache:masterfrom
AmiradelBeyg:fix-maxpendingmessages-default

Conversation

@AmiradelBeyg
Copy link
Copy Markdown
Contributor

Summary

ProducerBuilder.Create() rejects MaxPendingMessages = 0, but
PulsarClient.CreateProducer(ProducerOptions<TMessage>) did not.

This made producer creation behavior inconsistent depending on which API path was used. It also allowed an invalid
configuration to reach the internal send queue, which could hide problems until high-volume or production usage.

Changes

  • add missing MaxPendingMessages > 0 validation in PulsarClient.CreateProducer
  • keep the existing builder behavior unchanged
  • add unit coverage for both producer creation paths

Why

Both producer creation APIs should enforce the same contract. Rejecting invalid configuration early is clearer and
prevents subtle runtime issues caused by a zero-capacity pending message limit.

Testing

  • added unit test for PulsarClient.CreateProducer(ProducerOptions<TMessage>)
  • added unit test for ProducerBuilder.Create()
  • ran:
dotnet test tests/DotPulsar.Tests/DotPulsar.Tests.csproj --filter "PulsarClientProducerValidationTests|
ProducerBuilderTests"

## Notes

I did not add an integration test for MaxPendingMessages because there was no existing integration coverage for this
option, and the bug is a validation/contract issue at the API boundary.

@AmiradelBeyg AmiradelBeyg marked this pull request as ready for review March 11, 2026 21:01
ProducerBuilder.Create validated
  MaxPendingMessages, but PulsarClient.CreateProducer accepted 0 through ProducerOptions.

  This allowed an invalid producer configuration to reach the internal send queue, making the behavior inconsistent
  depending on which API was used to create the producer.

  Add the missing validation to PulsarClient.CreateProducer and add unit tests for both creation paths.
@AmiradelBeyg AmiradelBeyg force-pushed the fix-maxpendingmessages-default branch from c1b9d17 to 00032aa Compare March 11, 2026 21:13
Copy link
Copy Markdown
Contributor

@blankensteiner blankensteiner left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Two small corrections and I can merge it :-)

using DotPulsar.Exceptions;

[Trait("Category", "Unit")]
public sealed class PulsarClientProducerValidationTests
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This class should just be called "PulsarClientTests", otherwise everything is fine :-)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can rename that class, but there is already an existing integration test class named PulsarClientTests in this test project. Do you want me to rename the new unit test class and also rename the existing integration class, or would you prefer I keep the current name to avoid duplicate types?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The convention is that if we have a class called DotPulsar.SomeNamespace.ClassName, then we can find all tests for that class in DotPulsar.Tests.SomeNamespace.ClassNameTests, so if there is already a class with that name, then just add the new tests to that class :-)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

using DotPulsar.Extensions;

[Trait("Category", "Unit")]
public sealed class ProducerBuilderTests
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This class should be moved to the "Internal" namespace :-)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

@blankensteiner blankensteiner merged commit a6ebfab into apache:master Mar 19, 2026
3 checks passed
@blankensteiner
Copy link
Copy Markdown
Contributor

Thanks! :-)

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