Skip to content

AllowStale and RequireConsistent are mutually exclusive.#1500

Merged
tomwilkie merged 1 commit into
masterfrom
consul-stale
Jul 10, 2019
Merged

AllowStale and RequireConsistent are mutually exclusive.#1500
tomwilkie merged 1 commit into
masterfrom
consul-stale

Conversation

@tomwilkie
Copy link
Copy Markdown
Contributor

If you specify RequireConsistent: false, then you actually saying use the default - which is consistent. You need to set AllowState: true.

Signed-off-by: Tom Wilkie tom.wilkie@gmail.com

If you speficy RequireConsistent: false, then you actually saying use the default - which is consistent. You need to set AllowState: true.

Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>
@bboreham
Copy link
Copy Markdown
Contributor

I can see that during ordinary reads you don't mind seeing out-of-date info from other ingesters, but when you insert one into the ring that should be a real CAS, no?

@tomwilkie
Copy link
Copy Markdown
Contributor Author

tomwilkie commented Jul 10, 2019

The CAS will always be consistent, yeah.

More: the read before the CAS can be stale, then the CAS will fail. The idea is that eventually it will succeed. Whats more with a single consul node, there is no such thing as a stale read. If you run a clustered consul you probably want consistent reads.

@tomwilkie
Copy link
Copy Markdown
Contributor Author

tomwilkie commented Jul 10, 2019

Didn't see a huge impact from this change on consul CPU in dev; but they aren't under much load. Still worth it IMO. Reduced consul CPU by ~30% on a busy cluster.

@gouthamve
Copy link
Copy Markdown
Contributor

    // AllowStale allows any Consul server (non-leader) to service
    // a read. This allows for lower latency and higher throughput
    AllowStale bool

    // RequireConsistent forces the read to be fully consistent.
    // This is more expensive but prevents ever performing a stale
    // read.
    RequireConsistent bool

Why are there two bools to control one thing?

Copy link
Copy Markdown
Contributor

@gouthamve gouthamve left a comment

Choose a reason for hiding this comment

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

See comment. Otherwise LGTM

@gouthamve
Copy link
Copy Markdown
Contributor

Explained offline that the consistent reads will use raft log while stale ones dont.

@tomwilkie
Copy link
Copy Markdown
Contributor Author

tomwilkie commented Jul 10, 2019

Why are there two bools to control one thing?

I couldn't say, but the docs say:

To switch these modes, either the stale or consistent query parameters should be provided on requests. It is an error to provide both.

If you look at the code, setting either to true adds a parameter to the HTTP call:

https://github.com/armon/consul-api/blob/eb2c6b5be1b66bab83016e0b05f01b8d5496ffbd/api.go#L168-L173

	if q.AllowStale {
		r.params.Set("stale", "")
	}
	if q.RequireConsistent {
		r.params.Set("consistent", "")
	}

@tomwilkie tomwilkie merged commit 43718a4 into master Jul 10, 2019
@tomwilkie tomwilkie deleted the consul-stale branch July 10, 2019 18:53
@bboreham
Copy link
Copy Markdown
Contributor

bboreham commented Oct 1, 2020

More: the read before the CAS can be stale, then the CAS will fail. The idea is that eventually it will succeed.

Unfortunately the code has a fixed limit of 10 retries, and those complete in about 90ms on my system, so I re-assert that this is a bad idea. #3259

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants