Skip to content
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

Merged
merged 3 commits into from Jul 1, 2022

Conversation

arriven
Copy link
Contributor

@arriven arriven commented Jun 24, 2022

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

@arriven arriven requested review from a team as code owners June 24, 2022 11:15
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Jun 24, 2022
@google-cla
Copy link

google-cla bot commented Jun 24, 2022

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.

@product-auto-label product-auto-label bot added the api: pubsub Issues related to the Pub/Sub API. label Jun 24, 2022
@hongalex hongalex added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jun 27, 2022
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jun 27, 2022
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}
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Member

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!

@arriven arriven requested a review from hongalex July 1, 2022 06:42
@hongalex hongalex added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 1, 2022
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 1, 2022
@hongalex hongalex merged commit 71bd273 into googleapis:main Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pubsub/pstest: support message ordering
3 participants