Skip to content

Conversation

@nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Mar 4, 2022

Steps

  1. Create 70 users:
i=10
while [ $i -ne 70 ]
do
        i=$(($i+1))
        echo "$i"

	USERID=$(cat /proc/sys/kernel/random/uuid)
	OC_PASS="123456" occ -vvv user:add --password-from-env --display-name "Dummy #$i" -g "dummies" $USERID
	occ -vvv user:setting $USERID settings email "test$i@yourdomain.tld"

done
  1. Create a conversation as admin with group "dummies" (and check the database queries of this request)
  2. Post a message with @all

Query count

Message Before After
chat 16 16
@employee2 23 23
@all 367 90

Compare query log:
Bildschirmfoto von 2022-03-04 12-23-08

@nickvergessen nickvergessen added 3. to review bug feature: chat 💬 Chat and system messages feature: api 🛠️ OCS API for conversations, chats and participants performance 🚀 labels Mar 4, 2022
@nickvergessen nickvergessen added this to the 💛 Next Major (24) milestone Mar 4, 2022
@nickvergessen nickvergessen changed the title Inject room and attendee into notifier Inject room and attendee into chat notifier Mar 4, 2022
@nickvergessen nickvergessen requested a review from danxuliu March 4, 2022 11:32
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

Import of RoomNotFoundException should be removed in first commit and unit tests need to be fixed in second commit.

Other than the comments, code looks good (but I have not tested it).

Copy link
Contributor

@vitormattos vitormattos left a comment

Choose a reason for hiding this comment

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

I tested locally, working fine and get the same reducing of queries.

@nickvergessen
Copy link
Member Author

@danxuliu you fine with this step now?

@nickvergessen
Copy link
Member Author

Just asking because it would be nice to get this in and then continue with #6340

@danxuliu danxuliu force-pushed the techdebt/noid/inject-room-and-attendee-into-notifier branch from 58ff818 to 4397e63 Compare March 15, 2022 00:16
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

I split Vitor's commit in two fixups and added another fixup with the missing changes to the psalm docs.

In the end I did not replace if ($alreadyNotified === $userToNotify) { with if ($alreadyNotified['id'] === $userToNotify['id'] && $alreadyNotified['type'] === $userToNotify['type']) {. As explained, right now it should work fine, so let's adjust it in the future if the order in which the methods are called changes rather than preemptively adjust it now.

@nickvergessen nickvergessen force-pushed the techdebt/noid/inject-room-and-attendee-into-notifier branch from 4397e63 to c92c703 Compare March 15, 2022 08:40
@nickvergessen nickvergessen merged commit 40b59c6 into master Mar 15, 2022
@nickvergessen nickvergessen deleted the techdebt/noid/inject-room-and-attendee-into-notifier branch March 15, 2022 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release bug feature: api 🛠️ OCS API for conversations, chats and participants feature: chat 💬 Chat and system messages performance 🚀

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants