Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Make notifications at least once delivery #973

Merged
merged 1 commit into from
Mar 2, 2018
Merged

Conversation

Eochs
Copy link
Contributor

@Eochs Eochs commented Mar 2, 2018

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.

@Eochs Eochs requested review from squaremo, samb1729 and lelenanam March 2, 2018 01:32
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.

@samb1729
Copy link
Contributor

samb1729 commented Mar 2, 2018

Nitpick: that commit message is going to be hard to read 6 months from now.

I would suggest something like this:

Leave sync-tag unchanged if LogEvent returns an error

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.

Put the 'what' on the first line and then go into 'why' below, and leave the 'how' to the diff!

@samb1729
Copy link
Contributor

samb1729 commented Mar 2, 2018

Have you tested this change against a selectively-failing LogEvent handler?

How does the daemon's behaviour change if LogEvent constantly returns an error? I would guess without testing it myself that flux basically stops doing anything useful, but maybe that's not the case here, and maybe LogEvent always erroring should break things.

@squaremo
Copy link
Member

squaremo commented Mar 2, 2018

I would guess without testing it myself that flux basically stops doing anything useful

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.

@Eochs
Copy link
Contributor Author

Eochs commented Mar 2, 2018

@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.

Copy link
Contributor

@samb1729 samb1729 left a 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.
@Eochs Eochs force-pushed the 1550-at-least-once-events branch from 339ad9a to a634f17 Compare March 2, 2018 19:19
@Eochs Eochs merged commit cb551a4 into master Mar 2, 2018
@Eochs Eochs deleted the 1550-at-least-once-events branch March 2, 2018 19:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants