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

Order events by datetime instead of order #663

Closed
cortadocodes opened this issue Jun 26, 2024 · 3 comments · Fixed by #660
Closed

Order events by datetime instead of order #663

cortadocodes opened this issue Jun 26, 2024 · 3 comments · Fixed by #660
Assignees
Labels
bug Unintended behaviour in any area of the app

Comments

@cortadocodes
Copy link
Member

Bug report

What is the current behavior?

When getting events, both the question and the first response event from the child have order zero, so we get a duplicate event warning and can only see one of those events.

What is the expected behavior?

We should be able to get the question and the first response with no warning.

Proposed Solution

Order events by datetime in the event handlers instead of the order attribute.

@cortadocodes cortadocodes added the bug Unintended behaviour in any area of the app label Jun 26, 2024
@cortadocodes cortadocodes self-assigned this Jun 26, 2024
@cortadocodes
Copy link
Member Author

cortadocodes commented Jun 28, 2024

The only downside I can think of for this is that we lose the ability to know if:

  • Two messages are consecutive
  • There are missing messages
  • There are extra messages

I think it's worth it though as it will:

  1. Simplify the event handler quite a bit
  2. While still providing a good certainty in the order of the events (we control the datetime set in the events)
  3. Allow us to handle any group of events we like, even if they weren't emitted from the same source (e.g. the case of the question event plus the set of response events)

I think carrying on using the order attribute is actually tech-debt now that we've moved towards an event driven architecture.

@thclark
Copy link
Contributor

thclark commented Jun 28, 2024

I think on the basis that we don't ever, in practicality, actually do anything if we're missing messages, this is a small price to pay for a great simplification in an area where actually we've accumulated a lot of unnecessary complexity. So I'm all for this.

@cortadocodes cortadocodes linked a pull request Jul 1, 2024 that will close this issue
@cortadocodes
Copy link
Member Author

I've realised that, for synchronous questions, "ordering by datetime" would in fact mean just handling the events in the order that pub/sub sends them in. This is because datetimes are continuous whereas the integer order attribute was discrete.

We'll try this without a pub/sub ordering key and add one if it becomes an issue. Asynchronous questions can still be ordered by datetime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unintended behaviour in any area of the app
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants