[IMPROVED] Better attempt at delivering messages to queue subscriptions #638
Conversation
…ro max_msgs in a cluster
server/client.go
Outdated
| } | ||
| return | ||
| isRouteQsub = true | ||
| // for queue subscription try hard to deliver a message at least once. |
There was a problem hiding this comment.
We want to maintain strict at-most-once semantics. I do not think this violates it, but we should update the comment IMO.
There was a problem hiding this comment.
I guess you commented from original PR, which in this PR the comments were already changed. Anyway, this comment has been removed.
server/client.go
Outdated
| // we didn't make a delivery attempt, because either a subscriber limit | ||
| // was exceeded or a subscription is already gone. | ||
| // So, let a code below find yet another matching subscription. | ||
| // We are at risk that a message might go forth and back |
server/client.go
Outdated
| // So, let a code below find yet another matching subscription. | ||
| // We are at risk that a message might go forth and back | ||
| // between routes during these attempts, but at the end | ||
| // it shall either be delivered (at most once) or drop. |
server/client.go
Outdated
| // We are at risk that a message might go forth and back | ||
| // between routes during these attempts, but at the end | ||
| // it shall either be delivered (at most once) or drop. | ||
| c.Debugf("Re-sending message of a detached queue sid %s", c.pa.sid) |
There was a problem hiding this comment.
This causes concern here, I don't think we are re-sending a message here correct?
There was a problem hiding this comment.
Again, comment had been previously changed (and this debug statement removed).
server/client.go
Outdated
| c.deliverMsg(sub, mh, msg) | ||
| // Iterate over all subscribed clients starting at a random index | ||
| // until we find one that's able to deliver a message. | ||
| // Drop a message on the floor if there are noone. |
kozlovic
changed the title
|
@derekcollison I believe I have addressed the issues regarding comments. We need to think if we want this behavior to be merged or stick with the old one. I believe that this change still honor the at-most and there should not be duplicates. But original behavior was "correct" too (since we don't guarantee at-least-once, but at-most-once). |
|
As long as we can guarantee that we are still at most-once then LGTM. If we can not be sure of that then we need to revisit the changes. |
|
@derekcollison After careful review and to the best of my knowledge, as the code stands in this PR, we still honor at-most-once delivery but do a better job at delivery messages to a queue group. If you approve, please approve the PR (it is currently in Request Changes status). |
This PR is based out of #633. It imroves parsing QRSID so that the
TestRouteQueueSemantics test now passes (when dealing with malformed
QRSID).
A test similar to what is reported in #632 was also added. This
test however, uncovers a race condition that will be fixed in a
separate PR.
Resolves #632