Unify the replication calculation between query and push. #681

tomwilkie · 2018-02-02T10:06:48Z

Part of #604, fixes #279

The logic for doing replication was subtlety different on the query side and on the push side - push used the number of ingesters returned to calculate quorum, and query used the replication factor. Using the replication factor to calculate quorum will occasionally give the wrong answer when ingesters are joining and leaving. Whats more, there were a bunch of checks on the push path that weren't on the query path.

This PR unifies the logic between these two paths, effectively adding a bunch of checks on the query path and dealing with the case where you have less ingesters than the replication factor (by returning an appropriate error).

tomwilkie · 2018-02-05T14:54:06Z

(found a bug with this, don't merge just yet)

tomwilkie · 2018-02-06T11:17:57Z

After some more work, I realised all this replication logic really belongs to the ring, as does the replication factor itself. This reduces coupling quite a lot IMO, and the ring interface now becomes "tell me who to talk to and how many errors to tolerate", with all the liveness filtering and quorum calculations happening in the ring.

Going to deploy to our dev env and see how this works.

tomwilkie · 2018-02-06T12:19:06Z

Have put this through a couple of rolling upgrades and it works, I'd say its ready for review. @bboreham fancy this one?

bboreham

I found some nits.
I find this code rather confusing, will need to come back to it later.

bboreham · 2018-02-06T14:12:50Z

pkg/ring/ring.go

 		// size of the replica set for read, but we can read from Leaving ingesters,
 		// so don't skip it in this case.
+		// NB ingester will be filterer later (by distributor/replication_strategy).


spelling "filterer"

bboreham · 2018-02-06T14:13:23Z

pkg/ring/ring.go

-		// want to write the extra replica somewhere.  So we increase the size of the
-		// set of replicas for the key.  This means we have to also increase the
+		// We do not want to Write to Ingesters that are not ACTIVE, because they are
+		// about to go away, but we do want to write the extra replica somewhere.


Nit: could be pending or joining as well as leaving, so "about to go away" is not a full explanation.

bboreham · 2018-02-06T14:14:53Z

pkg/distributor/distributor_test.go

@@ -24,24 +24,33 @@ type mockRing struct {
 	heartbeatTimeout time.Duration


Do we still need this?

bboreham · 2018-02-06T14:21:53Z

pkg/ring/replication_strategy.go

+) {
+	// We need a response from a quorum of ingesters, which is n/2 + 1.  In the
+	// case of a node joining/leaving, the actual replica set might be bigger
+	// than the replication factor, so we need to account for this.


I don't think it is clear what "account for this" means or refers to.

bboreham · 2018-02-06T14:23:22Z

pkg/ring/replication_strategy.go

+	"time"
+)
+
+func (r *Ring) replicationStrategy(ingesters []*IngesterDesc) (


please explain what the overall purpose of this function is, and what the ingesters parameter should contain

tomwilkie · 2018-02-06T15:11:04Z

Thanks for the feedback; this is quite a complicated and important aspect of Cortex so a second brain is appreciated.

I've kept replicationStrategy as a separate function mainly for testing.

bboreham · 2018-02-08T15:26:55Z

AAMOI does this make it easier to do #467 (comment) ?

tomwilkie · 2018-02-08T15:37:36Z

Yes! We can reevaluate writing out the extra replica when we hit a non-active ingester, given our transfer protocol.

I'd prefer to fix up the transfers so they do the right thing though - instead of streaming of the new joining ingester, they should really calculate where their data should like once their tokens are gone, and send it there (similarly, joining nodes should 'steal' data from nodes based on the tokens they pick). This will enable us to much more easily scale up and down, and in this word the current logic in the ring is correct. But thats more work - I'll get to it this year I suspect.

tomwilkie · 2018-02-13T19:24:12Z

Friendly ping?

jml · 2018-02-14T10:56:52Z

@bboreham is not available this week. I'm not likely to get to this before then (although will try).

bboreham

It looks like the behaviour will change - we will no longer reads from LEAVING ingesters. Am I mistaken?

Also a couple more nits.

bboreham · 2018-02-20T11:23:59Z

pkg/ring/ring.go

-// The order of the result matches the order of the input.
-func (r *Ring) BatchGet(keys []uint32, n int, op Operation) ([][]*IngesterDesc, error) {
+// BatchGet returns ReplicationFactor (or more) ingesters which form the replicas
+// for the given key. The order of the result matches the order of the input.


bboreham · 2018-02-20T11:28:13Z

pkg/ring/ring.go

@@ -84,6 +94,10 @@ type Ring struct {

 // New creates a new Ring
 func New(cfg Config) (*Ring, error) {
+	if 0 > cfg.ReplicationFactor {
+		return nil, fmt.Errorf("ReplicationFactor must be greater than zero: %d", cfg.ReplicationFactor)


Error message does not match test. Suspect you meant 0 >=

bboreham · 2018-02-20T11:33:07Z

pkg/ring/replication_strategy.go

+
+// IsHealthy checks whether an ingester appears to be alive and heartbeating
+func (r *Ring) IsHealthy(ingester *IngesterDesc) bool {
+	if ingester.State != ACTIVE {


Looks like this means you no longer read from LEAVING ingesters - was that intended?

It wasn't, good catch.

bboreham · 2018-02-20T11:34:25Z

pkg/ring/ring.go

 		// size of the replica set for read, but we can read from Leaving ingesters,
 		// so don't skip it in this case.
+		// NB dead ingester will be filtered later (by replication_strategy.go).


dead and not-ACTIVE ingesters are filtered by IsHealthy(), which doesn't really match the logic here. Or I am missing the subtlety.

tomwilkie · 2018-02-20T12:45:00Z

Thanks for the review, I've reintroduced the old behaviour.

bboreham · 2018-02-28T12:18:50Z

pkg/ring/ring.go

@@ -293,7 +323,7 @@ func (r *Ring) Collect(ch chan<- prometheus.Metric) {
 		LEAVING.String(): 0,
 	}
 	for _, ingester := range r.ringDesc.Ingesters {
-		if !r.IsHealthy(ingester) {
+		if !r.IsHealthy(ingester, Write) {


Now you're never going to report LEAVING ingesters

bboreham · 2018-02-28T12:29:03Z

pkg/ring/http.go

@@ -102,7 +102,7 @@ func (r *Ring) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 		ing := r.ringDesc.Ingesters[id]
 		timestamp := time.Unix(ing.Timestamp, 0)
 		state := ing.State.String()
-		if !r.IsHealthy(ing) {
+		if !r.IsHealthy(ing, Write) {


probably don't want this to report unhealthy on LEAVING either

bboreham · 2018-03-07T12:00:02Z

@tomwilkie I think it's close; do you want to rebase and consider the reporting aspects?

…ategy is up to date.

bboreham · 2018-03-23T17:37:33Z

@tomwilkie I have rebased and added a suggested fix at https://github.com/weaveworks/cortex/compare/604-unify-replication-calcs; PTAL

tomwilkie · 2018-03-26T17:24:32Z

You're branch looks good Bryan; I've pull it in here. Thanks for doing that.

tomwilkie mentioned this pull request Feb 2, 2018

Distributor queries are weird when you have low numbers of ingesters #604

Closed

tomwilkie force-pushed the 604-unify-replication-calcs branch from 4ee5628 to df2f219 Compare February 2, 2018 11:57

bboreham reviewed Feb 6, 2018

View reviewed changes

bboreham suggested changes Feb 20, 2018

View reviewed changes

bboreham reviewed Feb 28, 2018

View reviewed changes

bboreham mentioned this pull request Mar 7, 2018

Stop early-return after majority success in distributor writes #732

Closed

tomwilkie and others added 12 commits March 23, 2018 17:13

Unify the replication calculation between query and push.

1f6268e

Replace 'circle' with 'ring'.

6fbf4b2

Deal with ingesters coming / going better.

bed8eb2

Fix comment.

cdfd837

Actually, all the replication calculations really belong in ring.

aa94d7d

Now we're only propagating MaxError, need to make sure replicationStr…

333f67a

…ategy is up to date.

Review feedback.

cf29050

Typo

23323c4

Fix nits

85e441c

Reintroduce old behaviour, of reading from LEAVING ingesters.

fb52bbb

Rebase fix-up

16112e4

Don't exclude some ingesters from metrics and http admin page

ace81ba

tomwilkie force-pushed the 604-unify-replication-calcs branch from 92048cd to ace81ba Compare March 26, 2018 17:23

bboreham approved these changes Mar 27, 2018

View reviewed changes

bboreham merged commit 7d6de67 into cortexproject:master Mar 27, 2018

tomwilkie deleted the 604-unify-replication-calcs branch July 13, 2018 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify the replication calculation between query and push. #681

Unify the replication calculation between query and push. #681

tomwilkie commented Feb 2, 2018 •

edited

Loading

tomwilkie commented Feb 5, 2018

tomwilkie commented Feb 6, 2018

tomwilkie commented Feb 6, 2018

bboreham left a comment

bboreham Feb 6, 2018

bboreham Feb 6, 2018

bboreham Feb 6, 2018

bboreham Feb 6, 2018

bboreham Feb 6, 2018

tomwilkie commented Feb 6, 2018

bboreham commented Feb 8, 2018

tomwilkie commented Feb 8, 2018 •

edited

Loading

tomwilkie commented Feb 13, 2018

jml commented Feb 14, 2018

bboreham left a comment

bboreham Feb 20, 2018

bboreham Feb 20, 2018

bboreham Feb 20, 2018

tomwilkie Feb 20, 2018

bboreham Feb 20, 2018

tomwilkie commented Feb 20, 2018

bboreham Feb 28, 2018

bboreham Feb 28, 2018

bboreham commented Mar 7, 2018

bboreham commented Mar 23, 2018

tomwilkie commented Mar 26, 2018

		@@ -24,24 +24,33 @@ type mockRing struct {
		heartbeatTimeout time.Duration

Unify the replication calculation between query and push. #681

Unify the replication calculation between query and push. #681

Conversation

tomwilkie commented Feb 2, 2018 • edited Loading

tomwilkie commented Feb 5, 2018

tomwilkie commented Feb 6, 2018

tomwilkie commented Feb 6, 2018

bboreham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomwilkie commented Feb 6, 2018

bboreham commented Feb 8, 2018

tomwilkie commented Feb 8, 2018 • edited Loading

tomwilkie commented Feb 13, 2018

jml commented Feb 14, 2018

bboreham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomwilkie commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboreham commented Mar 7, 2018

bboreham commented Mar 23, 2018

tomwilkie commented Mar 26, 2018

tomwilkie commented Feb 2, 2018 •

edited

Loading

tomwilkie commented Feb 8, 2018 •

edited

Loading