-
Notifications
You must be signed in to change notification settings - Fork 89
Added verification of the consumer sequence number for pull ordered consumers #981
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
Added verification of the consumer sequence number for pull ordered consumers #981
Conversation
cb1813e to
a34811b
Compare
mtmk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @sergeimam looks good. Just a comment about the concern you raised regarding a potential infinite loop when using max-bytes option. let me know what you think.
mtmk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks @sergeimam
|
|
||
| // Assert | ||
| Assert.True(result); | ||
| Assert.Equal(2, Interlocked.CompareExchange(ref _result, 0, 0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated test fix. rarely we get an error here. guessing potential race on the result bool.
| } | ||
| finally | ||
| { | ||
| var deleted = await TryDeleteConsumer(_fetchConsumerName, cancellationToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you care if the delete succeeds? It should be ephemeral anyway. The key is to always generate an ordered consumer name. We agreed that the user can provide a prefix for ordered consumer names, which will allow them to be found more easily in logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_fetchConsumerName is kept around to delete the ephemeral when we finish with it just to be a good citizen. so if we couldn't delete it this time round and there is another fetch we try our best to delete again potentially. I don't think prefix feature is implemented in this client.
scottf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Add prioritized mode (#1011) * Add Publish Timeout on Disconnect (#1001) * Fix test flap (#1012) * Add test to promote mirrored to regular stream (#1008) * Add Nats-Expected-Last-Subject-Sequence-Subject (#1007) * Fix keyed NATS clients with configurations (#1006) * ensure NatsConnectionPool cannot overflow (#1005) * Fix stream config adjustments on update (#995) * Handel Unobserved Exceptions During Connection State Transitions (#999) * Added verification of the consumer sequence number for pull ordered consumers (#981) * Fix build warnings (#991) * Reduce closure allocations (#988)
* Add prioritized mode (#1011) * Add Publish Timeout on Disconnect (#1001) * Fix test flap (#1012) * Add test to promote mirrored to regular stream (#1008) * Add Nats-Expected-Last-Subject-Sequence-Subject (#1007) * Fix keyed NATS clients with configurations (#1006) * ensure NatsConnectionPool cannot overflow (#1005) * Fix stream config adjustments on update (#995) * Handel Unobserved Exceptions During Connection State Transitions (#999) * Added verification of the consumer sequence number for pull ordered consumers (#981) * Fix build warnings (#991) * Reduce closure allocations (#988)
As per #980 added explicit verification of the consumer sequence for an ordered consumer