DEV: Use a single message bus subscription for topic tracking state. (PR #13989)

GitHub

I like this, but I am curious if there is some context/discussion behind this change? I like to link it to commits so we can find it in the future.

Additionally it seems like tests are failing.

Agree with Robin, I feel uneasy when I see empty PR descriptions, would be good to have some context and even some before/after measurements if this is a perf fix.

Hmm this is odd, the commit message provided more context but didn’t appear in the PR’s description. Either way, there are no measures needed here. This is simply removing the number of message bus subscriptions on the client side when a single subscription is sufficient.

I am not familiar with how message bus works; would this create problems with too many messages coming through the same channel, causing delays? Or is the order guaranteed, and messages delivered ASAP? This might not be an issue at all just thinking out loud, the new version is certainly a lot cleaner.

Just have a few questions, and the tests are breaking

Why make this a self method instead of a const?

It doesn’t seem right to me that we seem to have lost the user for the unread channel, shouldn’t that still be there?

It doesn’t seem right to me that we seem to have lost the user for the unread channel, shouldn’t that still be there?

The title of this pull request changed from “PERF: Use a single message bus subscription for topic tracking state.” to "DEV: Use a single message bus subscription for topic tracking state.

This is how messages are processed on the client side: message_bus/message-bus.js at 8acb2ed5721401c4bea2fe14f65132137d68389b · discourse/message_bus · GitHub

The messages are looped through all the channels subscription on the client side and the callback function is called if the message’s channel matches that of the callback. When we use less channels, each message has to loop through less channels before being processed. This does raise an interesting point as to why we don’t use a Map as the data store instead since channels on the client side are meant to be unique.

Good point I’ve made the change.

This is a concern to me, we are moving everything to one channel here which is not ideal, all sorts of things can break, backlog can start behaving in non ideal ways.

I am fine centralizing on /topic-tracking-state/USER_ID that way each user gets 1 dedicated channel with far more predictable backlogs and so on.

This is a good point. I’ll update the PR to use /topic-tracking-state/USER_ID instead.

Ahh ok I see why we can’t just split using the user_id because we don’t always have a user_id to publish to on the server side. For example, /latest is published to both anon and logged in users. What about increasing the backlog for the channel instead? Technically, all the channels before centralisation has the potential to burst the backlog too if the site experiences high levels of user activity.

Hmm this is odd, the commit message provided more context but didn’t appear in the PR’s description. Either way, there are no measures needed here. This is simply reducing the number of message bus subscriptions on the client side when a single subscription is sufficient.

I think this has turned into a good example of why we should have context/discussion around things like this! If we’d merged as is it would have been problematic. Should we close this in favor of our internal discussions?

@eviltrout I’m not sure I follow. The discussions were a result of the changes I proposed here. There were no context or discussions prior which had to be share. I had also intentionally tagged two reviewers who were familiar with the topic tracking state code so I would have at least gotten two approvals before merging. The fact that we caught the problem with the proposed changes is a good sign because it shows our code review process working.

Hi @tgxworld - I just was trying to reiterate that having a description with a detailed explanation for why a change is being made (or even better, a link to a topic) is very important.

It’s OK to use the PR as such as discussion, but I highly recommend making it a draft in that case.

Back to the drawing board on this one.