dendrite icon indicating copy to clipboard operation
dendrite copied to clipboard

Refactor notifications

Open S7evinK opened this issue 3 years ago • 2 comments

This PR changes the handling of notifications

  • removes the StreamEvent stream (todo: actually remove the stream from NATS)
  • listens on the OutputRoomEvent stream in the UserAPI to inform the SyncAPI about unread notifications

S7evinK avatar Aug 31 '22 15:08 S7evinK

What is this refactor trying to solve or achieve? I don't see a linked issue. I personally see two problems with the sync

  • the notifications and rooms have different stream positions, but we only send notifications to the client when there happen to be updates to both (almost never because of the way the threads line up) and we end up sending 0, 0 notification numbers when a different number exists in the sync db
  • you can't clear old notifications because we clean up the user notification db, but rely on the existence of a specific eventId in that db to forward on a message to the sync db where we store the aggregate.

I fixed both here, https://github.com/matrix-org/dendrite/pull/2701/files, but I'm lacking a lot of the bigger picture. I would love to hear your feedback on these two issues and understand what else you're trying to fix.

texuf avatar Sep 16 '22 15:09 texuf

It's basically trying to solve the same issue you are trying to, that the notification counts are not getting updated "in time"/correctly. The reason for this still being a draft is, that there's still something odd with clearing notifications, like you've mentioned. Good catch with the updated check, will see if that works better here.

With this PR the different stream positions would "disappear", there would only be one in the SyncAPI.

S7evinK avatar Sep 19 '22 05:09 S7evinK

I'm seeing tons of sql: converting argument $1 type: unsupported type []interface {}, a slice of interface after merging this pr,

level=error msg="GetUserUnreadNotificationCountsForRooms failed" device_id=JVIH5pVr error="sql: converting argument $1 type: unsupported type []interface {}, a slice of interface" limit=20"

Is this something I've messed up locally?

texuf avatar Sep 27 '22 22:09 texuf

Wow this is really broken, I'll put up a PR.

texuf avatar Sep 27 '22 23:09 texuf