-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat(pubsub/pstest): subscription message ordering #6257
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
orderingKey = id | ||
} | ||
if val, ok := orderingKeyMap[orderingKey]; !ok || m.proto.Message.PublishTime.AsTime().Before(val.m.proto.Message.PublishTime.AsTime()) { | ||
orderingKeyMap[orderingKey] = msg{m: m, id: id} |
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.
I'm a little confused as to how this works. If there are multiple messages with the same ordering key, doesn't this just override the map at that ordering key? Or is this intention only to deliver 1 message per ordering key at a time?
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.
Yes, it was intended. Alternative option would be to sort the collection accordingly but I'm not sure if that would work properly with multiple clients reusing the same subscription (AFAIU it's a valid use case and messages with the same ordering key should still not be processed in parallel) so I went ahead with a safer option as my first choice
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.
I'm assuming that real pubsub just makes sure that any consecutive messages with the same ordering key get delivered to the same client until all the messages with that key get acknowledged/deleted by retention (I actually want to test it out now) but I feel like replicating that behavior here would require a lot more changes and may not be suitable for a testing library.
One of the downsides of this particular approach is that it would be harder for you to ensure that client library correctly handles the case where multiple messages with the same ordering key get delivered to a client but since there are official client libraries in other languages I'm assuming that testing process for those are a lot more robust
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.
Regardless of that I'd be happy to change the approach if you think something else works better
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.
I'm assuming that real pubsub just makes sure that any consecutive messages with the same ordering key get delivered to the same client until all the messages with that key get acknowledged/deleted by retention
Yes, there is subscriber affinity in Pub/Sub with ordering keys but also agree this is probably out of the scope of the fake. I also think that testing subscriber affinity is something that is out of scope for testing in the client library so I think this approach works well. Thanks for authoring this change!
Fix #6255
It isn't the most effective solution (something like storing
map[orderingKey || id]struct{id, *message}
would be more effective) but considering that the package is intended to use for unit tests I think that lower risk outweighs the performance penalty. I would be happy to update it to a more effective solution if needed