-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Make notifications at least once delivery #973
Conversation
daemon/loop.go
Outdated
@@ -362,11 +362,13 @@ func (d *Daemon) doSync(logger log.Logger) (retErr error) { | |||
}, | |||
}); err != nil { | |||
logger.Log("err", err) | |||
return err |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
Nitpick: that commit message is going to be hard to read 6 months from now. I would suggest something like this:
Put the 'what' on the first line and then go into 'why' below, and leave the 'how' to the diff! |
Have you tested this change against a selectively-failing How does the daemon's behaviour change if |
It will keep syncing new things, but not report all events. It would look like the recent breakage (#967), since it'll fail at about the same stage -- after syncing, before moving the high water mark. |
@Sambooo since all of the sync logic happens before events are sent, if LogEvent fails, syncs will happen, but events will not be sent, and the sync-tag will not be moved. Then LogEvent will retry after every loop of sync. |
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.
LGTM, but rebase away the merge commit before actually merging.
We want to ensure at-least-once delivery of events to Weave Cloud, so if notifying the upstream fails we retry on the next sync. The task of deduplicating events is left to the consumer.
339ad9a
to
a634f17
Compare
changed flux loop so that if a sync or noteEvent fails to send, just return and do not move tag forward. This way if a notification fails, we wait for the next sync event to retry sending logic.
fixes weaveworks/service#1550
As stated in:
weaveworks/service#1550
Since fluxd is constantly checking and bringing the cluster's state in line with the specified state in git, if an event does not fire for one sync, it will likely fire on the next sync.