Queues: Move Notification to use a Queue #9902

zeripath · 2020-01-20T21:10:24Z

Move the notifier system to use a Queue.

First step is to move the ui service to use a Queue
Next in preparation to use a Queue we ensure that we can json marshal things passed in the notification
Autogeneration is used to create a notifier that wraps all the notification calls to a queue push and expand them back out
A special notification helper is provided for tests allowing these to specifically check notification events

zeripath · 2020-01-20T21:16:43Z

This one is potentially quite controversial ... Basically every request can cause a notification so this one might require a large amount of workers if we don't want it to get overloaded.

guillep2k · 2020-01-20T21:55:52Z

This one is potentially quite controversial ... Basically every request can cause a notification so this one might require a large amount of workers if we don't want it to get overloaded.

He he... 😁

Well, I'm in favor of longer queues rather than many workers. If we've got to send 1,000 mails or insert as many UI notifications, I'd rather do it in one or two workers than in 1,000. Heck! Our default db pool size is unlimited, which is not very friendly.

And since all workers would be potentially competing for the same resources, I think less workers would eventually be much faster if the number of items is sufficiently large. Maybe with this PR it will be easier to test, if I generate a large number of users. 🤔

2¢

zeripath · 2020-01-20T22:23:29Z

So there's still a race in the pull merge test. I'll have to do a bit more work on that.

I guess it's just enforce that what we're seeing is actually a merge or not for this test.

zeripath · 2020-01-20T22:27:14Z

@guillep2k I agree, we should grow worker pools where we know that they can cope with the concurrency and restrict the others with a large enough channel (for channel based queues.)

guillep2k

Oh, man, you've been having fun! 😁

modules/notification/base/main.go

integrations/notification_helper_test.go

modules/notification/base/main.go

guillep2k · 2020-01-21T02:46:58Z

modules/notification/base/notifier.go

@@ -9,6 +9,8 @@ import (
 "code.gitea.io/gitea/modules/repository"
 )

+//go:generate go run -mod=vendor main.go


So if any functions are added to the interface we need to run go generate and commit what's been created? Maybe the generated files should be added to .gitignore, but then changes in main.go become very difficult to review.

Yeah or just make build as we run go generate in build now.

notifier.go needs a comment to this effect though.

Added a few comments

Why we need to generate that two files? Any advantage the new method than before?

If you change notifier.go they will be both be kept up to date.

It would be beyond tiresome to write the notifier queue by hand and incredibly difficult to keep up to date.

guillep2k · 2020-01-21T04:05:21Z

models/issue.go

@@ -47,7 +47,7 @@ type Issue struct {
 IsClosed bool `xorm:"INDEX"`
 IsRead bool `xorm:"-"`
 IsPull bool `xorm:"INDEX"` // Indicates whether is a pull request or not.
- PullRequest *PullRequest `xorm:"-"`
+ PullRequest *PullRequest `xorm:"-" json:"-"`


I'm wondering whether attachments, comments, reactions, assignees, etc. should be skipped too. Because there are reference fields in those structs as well (isn't there an Issue property in Comment?). Sounds like recursions can cause some headaches. On the other hand, the serialized data is a snapshot of the object state that protects the notifier from other processes changing the data, so it makes some sense to keep it as is. Decisions, decisions... 🤔

yup. I decided to bridge the problem as and when we see it. The trouble is under the default testing case you might not see the issue as we don't necessarily try to jsonify these things. (Hmm maybe I should add a jsontestqueue that will always json.Marshal stuff.)

As we pass into notifier pull requests and issues which definitely have a cyclic reference and therefore MarshalJSON doesn't work - we need to break the cycle. Every PR has an issue. Not all issues have PRs - so breaking the cycle at issue seemed best.

I'm not particularly worried about cyclic references; those should stand out soon enough. But I think the overhead of serializing the whole bunch might be excessive. What I'm thinking is to use the opposite strategy: pass on only what is not derived, unless there's a reason to.

On the other hand, passing data which is tightly-coupled with the running Gitea version poses a problem for upgrading if any queues contain data during the process (e.g. file-system backed queues retained across shutdowns). One possible solution to this problem is to prevent upgrading if any queues contain data, but that may require a migration step being aware of the queue configuration and it would be very difficult to implement. May be we could drop a record on the database when shutting down if all queues are empty and the shutdown was orderly? A migration step could refuse to continue if that record is missing.

The trouble with not passing "derived" data is that the notification you may get may be different from what was done.

You have to pass and parse all the data that makes the notification because you can't be certain what's going to change in future.

If you migrate and you can't deserialize mostly the notification will be dropped or default values added.

As you say we need a flush mechanism. That may mean opening a non listening Gitea to run through its queues.

integrations/notification_helper_test.go

integrations/pull_merge_test.go

lunny · 2020-01-22T01:49:19Z

For test with notification, I think we just need a mockNotification and register only this notification. That will make the code simpler.

zeripath · 2020-01-26T18:58:28Z

I've forcibly rebased on #10001 as this should fix a number of issues with the tests. Except it doesn't because there is a race between the TestPullRequests and Merging.

codecov-io · 2020-02-10T17:30:24Z

Codecov Report

Merging #9902 into master will decrease coverage by 0.15%.
The diff coverage is 43.9%.

@@            Coverage Diff             @@
##           master    #9902      +/-   ##
==========================================
- Coverage   43.74%   43.58%   -0.16%     
==========================================
  Files         585      586       +1     
  Lines       81023    82598    +1575     
==========================================
+ Hits        35440    36002     +562     
- Misses      41202    42092     +890     
- Partials     4381     4504     +123

Impacted Files	Coverage Δ
models/user.go	`49.54% <ø> (ø)`	⬆️
models/issue.go	`53.95% <ø> (ø)`	⬆️
modules/convert/pull.go	`72.58% <0%> (+1.5%)`	⬆️
modules/notification/indexer/indexer.go	`56.96% <100%> (+2.29%)`	⬆️
routers/api/v1/repo/issue_comment.go	`57.39% <100%> (+0.18%)`	⬆️
modules/notification/notification.go	`79.85% <100%> (+0.59%)`	⬆️
modules/notification/mail/mail.go	`29.62% <33.33%> (+0.64%)`	⬆️
modules/notification/base/queue.go	`42.61% <42.61%> (ø)`
models/repo_unit.go	`72.64% <56.09%> (-10.44%)`	⬇️
modules/notification/ui/ui.go	`77.77% <60%> (+7.4%)`	⬆️
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17445bb...a7ad2ab. Read the comment docs.

routers/api/v1/repo/issue_comment.go

modules/notification/ui/ui.go

6543 · 2020-03-31T12:12:54Z

can you pleace resolve confilcts :D

lafriks · 2020-04-27T21:28:32Z

@zeripath ping

Signed-off-by: Andrew Thornton <[email protected]>

lunny · 2020-05-02T09:39:31Z

We have to be careful most notifier function params are pointers that maybe changed both in the notifiers' go routines or the original go routine.

zeripath · 2020-05-16T21:23:44Z

This should not go in to 1.12 at this stage.

codecov-commenter · 2020-06-19T07:04:23Z

Codecov Report

Merging #9902 into master will decrease coverage by 0.25%.
The diff coverage is 43.80%.

@@            Coverage Diff             @@
##           master    #9902      +/-   ##
==========================================
- Coverage   43.88%   43.62%   -0.26%     
==========================================
  Files         614      608       -6     
  Lines       87607    88522     +915     
==========================================
+ Hits        38449    38622     +173     
- Misses      44421    45070     +649     
- Partials     4737     4830      +93

Impacted Files	Coverage Δ
models/issue.go	`51.94% <ø> (+0.37%)`	⬆️
models/user.go	`49.66% <ø> (-1.38%)`	⬇️
modules/convert/pull.go	`70.33% <0.00%> (+1.64%)`	⬆️
modules/notification/mail/mail.go	`30.23% <33.33%> (+0.50%)`	⬆️
modules/notification/base/queue.go	`42.39% <42.39%> (ø)`
models/repo_unit.go	`72.64% <56.09%> (-10.44%)`	⬇️
modules/notification/base/null.go	`69.69% <68.75%> (ø)`
modules/notification/indexer/indexer.go	`56.96% <100.00%> (+6.96%)`	⬆️
modules/notification/notification.go	`80.41% <100.00%> (+0.56%)`	⬆️
modules/notification/ui/ui.go	`80.76% <100.00%> (+5.12%)`	⬆️
... and 90 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aaaaeb7...d43d378. Read the comment docs.

6543 · 2020-09-02T15:21:40Z

pleace resolve conflict

zeripath added the type/enhancement An improvement of existing functionality label Jan 20, 2020

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 20, 2020

lunny added this to the 1.12.0 milestone Jan 21, 2020

guillep2k reviewed Jan 21, 2020

View reviewed changes

guillep2k approved these changes Jan 22, 2020

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 22, 2020

zeripath force-pushed the queues-notification branch from 997e66a to e7e48ff Compare January 26, 2020 18:57

zeripath force-pushed the queues-notification branch from e7e48ff to 62e8d86 Compare January 27, 2020 17:03

zeripath force-pushed the queues-notification branch 3 times, most recently from aefbb99 to 13ba0da Compare February 10, 2020 17:07

lunny reviewed Feb 11, 2020

View reviewed changes

routers/api/v1/repo/issue_comment.go Outdated Show resolved Hide resolved

lunny reviewed Feb 11, 2020

View reviewed changes

modules/notification/ui/ui.go Outdated Show resolved Hide resolved

zeripath mentioned this pull request Feb 15, 2020

Notification: queue ui.go notification-service #10281

Merged

zeripath added 7 commits February 19, 2020 13:21

Ensure things are json marshalable

5d8a52c

Notification: move to use a queue

7138650

Add some more comments

d313a4e

Improve pull_merge_test

7e99301

Sort func names

569c7d7

Ensure fork has unique name in pull_merge_test

e090f8e

Remove unnecessary load of assignee

a7ad2ab

zeripath force-pushed the queues-notification branch from 8ef2b31 to a7ad2ab Compare February 19, 2020 13:24

Merge remote-tracking branch 'origin/master' into queues-notification

1132cb2

zeripath force-pushed the queues-notification branch from b32a83e to 1132cb2 Compare April 30, 2020 18:39

zeripath and others added 3 commits April 30, 2020 19:54

placate lint

c21e6e9

Signed-off-by: Andrew Thornton <[email protected]>

Merge branch 'master' into queues-notification

d288f3b

Merge branch 'master' into queues-notification

1db4c91

lafriks approved these changes May 16, 2020

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 16, 2020

lafriks added 2 commits May 17, 2020 00:07

Merge branch 'master' into queues-notification

8b84b1c

Merge branch 'master' into queues-notification

d43d378

zeripath added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label May 16, 2020

zeripath modified the milestones: 1.12.0, 1.13.0 May 16, 2020

lunny modified the milestones: 1.13.0, 1.14.0 Sep 10, 2020

lunny modified the milestones: 1.14.0, 1.x.x Jan 27, 2021

zeripath closed this Oct 15, 2022

lunny removed this from the 1.x.x milestone Oct 17, 2022

go-gitea locked and limited conversation to collaborators May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Queues: Move Notification to use a Queue #9902

Queues: Move Notification to use a Queue #9902

zeripath commented Jan 20, 2020 •

edited

Loading

zeripath commented Jan 20, 2020

guillep2k commented Jan 20, 2020

zeripath commented Jan 20, 2020

zeripath commented Jan 20, 2020

guillep2k left a comment

guillep2k Jan 21, 2020

zeripath Jan 21, 2020

zeripath Jan 21, 2020

lunny Jan 22, 2020

zeripath Jan 22, 2020

guillep2k Jan 21, 2020

zeripath Jan 21, 2020

guillep2k Jan 22, 2020

zeripath Jan 22, 2020

lunny commented Jan 22, 2020

zeripath commented Jan 26, 2020 •

edited

Loading

codecov-io commented Feb 10, 2020 •

edited

Loading

6543 commented Mar 31, 2020

lafriks commented Apr 27, 2020

lunny commented May 2, 2020 •

edited

Loading

zeripath commented May 16, 2020

codecov-commenter commented Jun 19, 2020

6543 commented Sep 2, 2020

Queues: Move Notification to use a Queue #9902

Queues: Move Notification to use a Queue #9902

Conversation

zeripath commented Jan 20, 2020 • edited Loading

zeripath commented Jan 20, 2020

guillep2k commented Jan 20, 2020

zeripath commented Jan 20, 2020

zeripath commented Jan 20, 2020

guillep2k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Jan 22, 2020

zeripath commented Jan 26, 2020 • edited Loading

codecov-io commented Feb 10, 2020 • edited Loading

Codecov Report

6543 commented Mar 31, 2020

lafriks commented Apr 27, 2020

lunny commented May 2, 2020 • edited Loading

zeripath commented May 16, 2020

codecov-commenter commented Jun 19, 2020

Codecov Report

6543 commented Sep 2, 2020

zeripath commented Jan 20, 2020 •

edited

Loading

zeripath commented Jan 26, 2020 •

edited

Loading

codecov-io commented Feb 10, 2020 •

edited

Loading

lunny commented May 2, 2020 •

edited

Loading