Skip to content
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

reset watch's lastIndex on error #2621

Merged
merged 3 commits into from
Jun 2, 2017
Merged

reset watch's lastIndex on error #2621

merged 3 commits into from
Jun 2, 2017

Conversation

alicebob
Copy link

Watch()ing a agent in -dev mode misses changes when the agent restarts.

When a -dev agent is restarted it'll have a clean state, including a
reset index. A watch() will reconnect after a restart of the agent, but it won't
notice that the index counter has reset and it will keep waiting until
the server reached the old index again, which is wrong. Resetting the index
prevents that and makes watch() work for -dev agents over restarts.

When a -dev agent is restarted it'll have a clean state, including a
reset index. A watch() will reconnect after a restart, but it won't
notice that the index counter has reset and it will keep waiting until
we reached the old index again, which is wrong. Resetting the index will
prevent that and makes watch work for -dev agents.
Copy link
Contributor

@slackpad slackpad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alicebob thanks for opening a PR - I think we need a tiny logic update to catch all the cases.

@@ -57,6 +57,7 @@ OUTER:
if err != nil {
// Perform an exponential backoff
failures++
p.lastIndex = 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this'll catch all cases since you won't always get an error. It's probably better to add something down on line 88 like this:

if p.lastIndex < oldIndex {
    p.lastIndex = 0
}

This'll reset the blocking query whenever the index slips backwards.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your reply, @slackpad.

The original fix I can easily reproduce in a test setup (watch() something and restart the Consul server in -dev mode), but a index which goes back (but doesn't drop the connection) I can't test. So I've added the lines you suggested, but I can't test it.

I also changed my original fix to reset the index to 0, not to 1.

(if this is fine I'll squash all commits)

@slackpad slackpad added this to the 0.8.0 milestone Feb 8, 2017
@slackpad slackpad modified the milestones: 0.8.0, 0.8.1 Mar 30, 2017
@slackpad slackpad modified the milestones: 0.8.1, 0.8.2 Apr 12, 2017
@slackpad slackpad removed this from the 0.8.2 milestone Apr 25, 2017
@slackpad slackpad added type/enhancement Proposed improvement or new feature theme/internal-cleanup Used to identify tech debt, testing improvements, code refactoring, and non-impactful optimization labels May 25, 2017
@slackpad slackpad merged commit 5f9776a into hashicorp:master Jun 2, 2017
@alicebob
Copy link
Author

alicebob commented Jun 2, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/internal-cleanup Used to identify tech debt, testing improvements, code refactoring, and non-impactful optimization type/enhancement Proposed improvement or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants