object, incident: Remove runtime cache#463
Conversation
oxzi
force-pushed
the
incidents-and-objects-only-in-the-database
branch
from
d9a6cf6 to
b244bfa
Compare
oxzi
left a comment
oxzi
left a comment
There was a problem hiding this comment.
Thanks for the review, @yhabteab. I have tried to address all of your comments and pushed a rebased version.
I have rewritten the incident.ReevaluateEscalations function to separate the work into two goroutines: a producer and a consumer.
Unfortunately, I could not use db.YieldAll as it requires the database.Entity interface - requiring database.IDer, which requires func ID() ID, where ID requires String() -, while Incident already implements contracts.Incident - requiring func ID() int64. This contradicts and working around this would be more boilerplate than saving otherwise.
oxzi
force-pushed
the
incidents-and-objects-only-in-the-database
branch
2 times, most recently
from
03a905a to
2c5efce
Compare
|
Pushed an updated version, that addresses the |
oxzi
force-pushed
the
incidents-and-objects-only-in-the-database
branch
from
2c5efce to
53e397a
Compare
With switching from a single node to a multi-node HA setup, storing both objects and incidents locally and only synchronizing them once from the database results in a diverged state between different nodes. Now, the local cache is gone and both objects and incidents are directly fetched from the database. This required further changes, most prominently moving the incident reevaluation from a local timer to the database as well. So, a new next_escalation_check_at column was added to the incident table, which is scanned routinely. Another change is switching mutexes for transactions, using the "SELECT ... FOR UPDATE" pattern to lock the current incident.
oxzi
force-pushed
the
incidents-and-objects-only-in-the-database
branch
from
53e397a to
93dc144
Compare
|
|
||
| if len(escalations) == 0 { | ||
| var notifications []*NotificationEntry | ||
| var o *object.Object |
There was a problem hiding this comment.
I think, this variable pre-declaration isn't need anymore.
| if i.notifyContact(contact, ev, notification.ChannelID) != nil { | ||
| contactName := contact.String() | ||
| err := i.notifyContact(obj, contact, ev, notification.ChannelID) | ||
| i.runtimeConfig.RUnlock() |
There was a problem hiding this comment.
Instead of passing notifications.ChannelID as an argument and accessing the runtimeConfig there again, I would just pass the channel instead and release config lock before notifying.
| err := retry.WithBackoff( | ||
| ctx, | ||
| func(ctx context.Context) error { return i.ProcessEvent(ctx, ev) }, | ||
| retry.Retryable, | ||
| backoff.DefaultBackoff, | ||
| db.GetDefaultRetrySettings()) |
There was a problem hiding this comment.
I don't think wrapping the entire function call here in a retry is a good idea, since ProcessEvent can fail for a number of reasons including notifying users.
|
As discussed offline, I've pushed the changes in a separate branch, please have a look at it and incorporate into this PR as you like. |
2 participants
With switching from a single node to a multi-node HA setup, storing both objects and incidents locally and only synchronizing them once from the database results in a diverged state between different nodes. Now, the local cache is gone and both objects and incidents are directly fetched from the database.
This required further changes, most prominently moving the incident reevaluation from a local timer to the database as well. So, a new next_escalation_check_at column was added to the incident table, which is scanned routinely.
Another change is switching mutexes for transactions, using the "SELECT ... FOR UPDATE" pattern to lock the current incident.
Refs #394. Fixes #457.