-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Queues: Move Notification to use a Queue #9902
Conversation
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¢ |
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. |
@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.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, man, you've been having fun! 😁
@@ -9,6 +9,8 @@ import ( | |||
"code.gitea.io/gitea/modules/repository" | |||
) | |||
|
|||
//go:generate go run -mod=vendor main.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah or just make build as we run go generate in build now.
notifier.go needs a comment to this effect though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need to generate that two files? Any advantage the new method than before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
@@ -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:"-"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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... 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
For test with notification, I think we just need a mockNotification and register only this notification. That will make the code simpler. |
997e66a
to
e7e48ff
Compare
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. |
e7e48ff
to
62e8d86
Compare
aefbb99
to
13ba0da
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
8ef2b31
to
a7ad2ab
Compare
can you pleace resolve confilcts :D |
@zeripath ping |
b32a83e
to
1132cb2
Compare
Signed-off-by: Andrew Thornton <[email protected]>
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. |
This should not go in to 1.12 at this stage. |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
pleace resolve conflict |
Move the notifier system to use a Queue.
Closes #8874