Parallelise and short cut distributor queries. #278
Conversation
| if len(ingesters) < d.cfg.MinReadSuccesses { | ||
| return fmt.Errorf("could only find %d ingesters for query. Need at least %d", len(ingesters), d.cfg.MinReadSuccesses) | ||
| // We need a response from a quorum of ingesters, which is n/2 + 1. | ||
| minSuccess := (len(ingesters) / 2) + 1 |
There was a problem hiding this comment.
By my understanding of the DynamoDB paper, the constraint isn't that quorum is n/2 + 1, but rather that r + w > n, where r is minimum read quorum and w minimum write.
There was a problem hiding this comment.
s/DynamoDB/Dynamo/
But yes, that is what the paper says. In our case, we set r = w = n / 2 + 1 (although the +1 is due to integer arithmetic). So r+w > n -> 2r > n -> 2n/2 + 2 > n -> n + 2 > n
There was a problem hiding this comment.
TIL: "DynamoDB exposes a similar data model and derives its name from Dynamo, but has a different underlying implementation"
There was a problem hiding this comment.
Yeah, DynamoDB shares virtually no relation to Dynamo, save for name. Consistency and replication model is very different.
distributor/distributor.go
Outdated
| } | ||
| // Fetch samples from multiple ingesters | ||
| var numErrs int32 | ||
| errs := make(chan error) |
There was a problem hiding this comment.
The routine below is constructed such that this will only ever receive one error, so the name errs is misleading. tooManyErrs? errReceived? maxErr?
distributor/distributor.go
Outdated
| @@ -527,7 +543,7 @@ func (d *Distributor) forAllIngesters(f func(cortex.IngesterClient) (interface{} | |||
| numErrs++ | |||
| } | |||
| } | |||
| if numErrs > (d.cfg.ReplicationFactor - d.cfg.MinReadSuccesses) { | |||
| if numErrs > 1 { | |||
There was a problem hiding this comment.
I'd have expected this to be zero. Why is one failure OK but two not OK?
There was a problem hiding this comment.
So I guess this should really be floor(d.cfg.ReplicationFactor / 2) to keep the invariant above... which for RF=3 is 1. Will make it dynamic.
I've made an issue for unifying the query logic ala the schema PR: #279 |
Part of #209