pulsar-dotpulsar icon indicating copy to clipboard operation
pulsar-dotpulsar copied to clipboard

feat: #46 support nack delay and ack timeouts

Open dionjansen opened this issue 3 years ago • 5 comments

  • feat: #45 Consumer now supports NegativeAcknowledge
  • add AcknowledgementTimeout to ConsumerOptions
  • add NegativeAcknowledgementRedeliveryDelay to ConsumerOptions
  • add NegativeackedMessageState to manage nacked messages
  • add UnackedMessageState to manage unacked messages
  • add MessageTracker to periodically check unacked and nacked messages, on a fixed polling timeout of 10ms
  • add AwaitingAck to track both unacked and nacked messages
  • add InactiveMessageTracker to reduce overhead when no AcknowledgementTimeout or NegativeAcknowledgementRedeliveryDelay is configured
  • add InactiveNegativeackedMessageState to reduce overhead when no NegativeAcknowledgementRedeliveryDelay is configured
  • add InactiveUnackedMessageState to reduce overhead when no AcknowledgementTimeout is configured
  • update ConsumerBuilder to allow setting AcknowledgementTimeout
  • update ConsumerBuilder to allow setting NegativeAcknowledgementRedeliveryDelay
  • refactor ConsumerChannel to support NegativeAcknowledge
  • refactor AsyncQueue<T> to implement missing interface IAsyncQueue<T>
  • refactor BatchHandler<TMessage> to implement missing interface IBatchHandler<TMessage>
  • add AutoFixture and AutoFixture.AutoNSubstitute dependencies to unit test project
  • add missing ConsumerBuilderTests unit tests
  • add missing ConsumerChannelFactoryTests unit tests
  • add missing ConsumerChannelTests unit tests
  • add missing ConsumerTests unit tests
  • add IntegrationTests for consumer ack timout and nack delays
  • skipped integration test SinglePartition_WhenSendMessages_ThenGetMessagesFromSinglePartition to avoid CI failures
  • skipped integration test RoundRobinPartition_WhenSendMessages_ThenGetMessagesFromPartitionsInOrder to avoid CI failures

Closes: #46 Closes: #45

dionjansen avatar Jul 23 '21 16:07 dionjansen

@blankensteiner let me know what you think

dionjansen avatar Jul 23 '21 16:07 dionjansen

@blankensteiner so I was a bit worried about the interactions with batching, so I've added two integration tests where I attempt to create a scenario where this functionality is tested: https://github.com/apache/pulsar-dotpulsar/blob/e7df3a528e1dd948cd1ec23a5baa51d693fac34f/tests/DotPulsar.IntegrationTests/ConsumerTests.cs#L225-L226

Looking at the other PR's and the still open issue https://github.com/apache/pulsar-dotpulsar/issues/7, am I fooling myself that this tests actually batches the messages and that at the moment there is actually no way (yet) to produce messages in a batched way using dotpulsar? That would make testing this particular behaviour tricky. I'd need to rely on deep(er) knowledge of the workings of batching to implement the right handling for this.

dionjansen avatar Jul 25 '21 11:07 dionjansen

so I was a bit worried about the interactions with batching, so I've added two integration tests where I attempt to create a scenario where this functionality is tested:

https://github.com/apache/pulsar-dotpulsar/blob/e7df3a528e1dd948cd1ec23a5baa51d693fac34f/tests/DotPulsar.IntegrationTests/ConsumerTests.cs#L225-L226

Looking at the other PR's and the still open issue #7, am I fooling myself that this tests actually batches the messages and that at the moment there is actually no way (yet) to produce messages in a batched way using dotpulsar? That would make testing this particular behaviour tricky. I'd need to rely on deep(er) knowledge of the workings of batching to implement the right handling for this.

Thanks for your PR. Currently, dotpulsar does not yet support batch sending. The code in your test isn't the correct batched way. Can we handle this scenario after the batch support is added?

RobertIndie avatar Jul 26 '21 02:07 RobertIndie

Thanks for your PR. Currently, dotpulsar does not yet support batch sending. The code in your test isn't the correct batched way. Can we handle this scenario after the batch support is added?

@RobertIndie yes that's okay for me, though I would need a strategy (for this PR) to simulate a batched message (to read). Do you have any ideas?

dionjansen avatar Aug 25 '21 06:08 dionjansen

Is there a reason this has not been merged? NegativeAcknowledge is an important part of the messaging capabilities of Pulsar.

adminnz avatar Apr 02 '22 09:04 adminnz