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

Backgrounding of Notifications Domain #695

Merged
merged 15 commits into from
Apr 29, 2024
Merged

Conversation

amydevs
Copy link
Contributor

@amydevs amydevs commented Apr 8, 2024

Description

This PR applies backgrounding to the Notifications domain by using TaskManager.

The sending of notifications can be either specified as blocking, or non-blocking, and with any amount of retries.

By doing this, errors on attempts to send notifications are now caught and turned into warning logs instead for non-blocking notifications.

Methods and RPC handlers to list and cancel the pending notifications are also to be added.

Issues Fixed

Tasks

  • 1. Implement non-blocking sending of notifications.
  • 2. Implement outbox notification listing
  • 3. Implement outbox notification cancelling
  • 4. Tests for outbox notifications in NotificationsManager
  • 5. Tests for outbox notification RPC handlers

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Apr 8, 2024

I'm thinking that if you're going to structure the DB to have outbox, will you also be creating sublevels for inbox? I think we would want to keep our "async messaging" layer to a minimal functionality, let's not recreate email inside Polykey. So from an async messaging layer perspective, the inbox is just all the messages we have received. But an outbox is all the messages we have sent out. Both have their own IdSortable ids, and is sorted by time.

What's interesting is that outbound messages would only have our own local IdSortable - sender notification ID.

Whereas inbound messages would have both the sender notification ID and our own received notification ID.

Furthermore I want us all to realise (@MatrixAI/matrixai-engineers) that we have a sync layer, and now by wrapping certain operations into the task manager, we introduce an async layer, for delay tolerance.


This means notifications which used to be synchronous is entirely changed to be asynchronous, which is what it is intended to be.

So therefore - this level of architectural change is a major thing. @amydevs you should be writing up in the title and spec that this is changing how the notifications domain works entirely so that notification messaging is an asynchronously operated domain.

Other domains that are also natively asynchronous are the like the discovery domain - and the way we discover our gestalt graph.

@amydevs
Copy link
Contributor Author

amydevs commented Apr 12, 2024

@CMCDragonkai

Currently the outbox is driven by the state of the parameters of the Task that stores the Notification and the corresponding Notification ID. I did this to simplify the state, and not have to manage 2 sets of state that are not relational be about the same notification. Also, since we're not rlly implementing a full messaging system, I thought that history for sent notifications wouldn't rlly be needed, so using the TaskManager for the pending notification state would be fine. The only concern would be that starting a fresh TaskManager would explicitly empty the Tasks for outbox notifications, but that would implicitly delete all outbox notifications.

@amydevs amydevs force-pushed the feature-notifications branch 6 times, most recently from 55f3704 to 20dd528 Compare April 15, 2024 06:48
Copy link
Contributor

@tegefaulkes tegefaulkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CMCDragonkai Asked me to have a look over. Seems some things can be simplified and other things to be avoided. But otherwise looking fine.

I'll look deeper into the tests later.

src/client/handlers/NotificationsOutboxRead.ts Outdated Show resolved Hide resolved
src/notifications/NotificationsManager.ts Outdated Show resolved Hide resolved
src/notifications/NotificationsManager.ts Outdated Show resolved Hide resolved
src/notifications/NotificationsManager.ts Outdated Show resolved Hide resolved
src/notifications/NotificationsManager.ts Outdated Show resolved Hide resolved
src/notifications/NotificationsManager.ts Outdated Show resolved Hide resolved
src/notifications/NotificationsManager.ts Outdated Show resolved Hide resolved
src/notifications/NotificationsManager.ts Outdated Show resolved Hide resolved
src/notifications/NotificationsManager.ts Outdated Show resolved Hide resolved
src/notifications/NotificationsManager.ts Outdated Show resolved Hide resolved
@tegefaulkes
Copy link
Contributor

Oh, and be sure replace any implicit return types with explicit ones. The implicitness is generally something we want to avoid.

@amydevs amydevs force-pushed the feature-notifications branch 3 times, most recently from 82f998a to 77abc68 Compare April 18, 2024 07:00
@CMCDragonkai
Copy link
Member

I think you should keep the retry delay simple as a 2x factor. It's the same elsewhere. Any change to this would need to be a separate issue.

Also there will need to be a corresponding PR in the CLI too. So that issue needs to be specced out.

@amydevs amydevs force-pushed the feature-notifications branch 3 times, most recently from 0409937 to a0c99a5 Compare April 19, 2024 04:04
@CMCDragonkai
Copy link
Member

@CMCDragonkai

Currently the outbox is driven by the state of the parameters of the Task that stores the Notification and the corresponding Notification ID. I did this to simplify the state, and not have to manage 2 sets of state that are not relational be about the same notification. Also, since we're not rlly implementing a full messaging system, I thought that history for sent notifications wouldn't rlly be needed, so using the TaskManager for the pending notification state would be fine. The only concern would be that starting a fresh TaskManager would explicitly empty the Tasks for outbox notifications, but that would implicitly delete all outbox notifications.

Is this still the case? Task state shouldn't really be used for keeping track of notifications. It should keep track of tasks. Separation of concerns. The task itself can reference the notification though.

@amydevs amydevs force-pushed the feature-notifications branch 2 times, most recently from 607a73c to fce26df Compare April 19, 2024 05:48
@amydevs amydevs force-pushed the feature-notifications branch from f310eb3 to 87d7ac7 Compare April 26, 2024 01:21
Copy link
Contributor

@tegefaulkes tegefaulkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good.

src/client/handlers/NotificationsOutboxRead.ts Outdated Show resolved Hide resolved
@amydevs amydevs force-pushed the feature-notifications branch 4 times, most recently from eb46b06 to 9740784 Compare April 29, 2024 04:28
amydevs added 5 commits April 29, 2024 14:30
fix: exponential backoff for notifications

fix: outbox notifications are now given ids

fix: notification tasks are now no longer lazy

fix: sendNotification now returns notificationId

fix: outbox notifications now use DB state

fix: NotificationId is now included in Notification, with the id and senderId fields

fix: `NotificationsManager` `Notification`s now have different types for state in DB and in usage

fix: tests
… rather than blocking directly from the returned promise

fix: `NotificationsManager` tests
@amydevs amydevs force-pushed the feature-notifications branch 3 times, most recently from 138dbed to 82553ac Compare April 29, 2024 04:33
amydevs added 10 commits April 29, 2024 14:34
fix: removed idUtils toBuffer usage

fix: changed position of sendNotificationHandlerId

fix: notificationsManager sendProm name

fix: removed count from NotificationsManager

fix: simplified NotificationId to TaskId reference

fix: sendNotificationHandler logic

fix: NotificationsManager db levelpath docblocks

fix: defaultRetries on NotifciationsManager constructor
…turn

AsyncGenerator

fix: removed unneeded creation of transaction in private methods

fix: notificationsRead handler now uses asyncgenerator

fix: readded blocking to NotificationsSend handler

fix: tests and exports for notifications outbox handlers
`NotificationsRemove` RPC handlers
…ionsRemove` RPC calls to `notificationsInboxRead`, `notificationsInboxClear` and `notificationsInboxRemove` respectively
fix: `notifications.test.ts` for RPC handlers involving notification IDs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Backgrounding of NotificationsManagerfor Fault Tolerance
3 participants