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
Feat/query retry #242
Feat/query retry #242
Conversation
2b8a534
to
9780610
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested it in your local environment? It'd be nice if we could validate it with following topology:
- shard1-r1
- shard1-r2
- shard2-r1
- shard2-r2
and two strategies: - you kill one of the nodes and make sure the retry strategy works correctly when the queried node was the dead one (you may want to disable heartbeat's otherwise it'll be hard)
- you kill both replicas of shard 1 for instance. Query should be refused
config/config.go
Outdated
@@ -342,6 +345,9 @@ type Cluster struct { | |||
|
|||
// Catches all undefined fields | |||
XXX map[string]interface{} `yaml:",inline"` | |||
|
|||
// Retry number for query - how many times a query can retry after bad gateway response from the host | |||
RetryNumber int `yaml:"retrynumber"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to stay consistent with other fields and omit empty values
RetryNumber int `yaml:"retrynumber"` | |
RetryNumber int `yaml:"retry_number",omitempty` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're getting there! For the configuration change, we'll need to also update the doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're still missing a doc. Other than that and the minor comments I left it looks good !
@@ -203,16 +203,17 @@ func executeWithRetry( | |||
respondWith(srw, err1, srw.StatusCode()) | |||
return since, nil | |||
} else { | |||
h := s.host | |||
h.dec() | |||
atomic.StoreUint32(&h.active, uint32(0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have checked the load-balancer argo, we need to penalise the host when the query failed. Including the query that we want to retry.
req.URL.Scheme = s.host.addr.Scheme | ||
log.Debugf("the valid host is: %s", s.host.addr) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, this if condition is not required in the real production env.
Description
Addresses #185
Pull request type
Please check the type of change your PR introduces:
Checklist
Does this introduce a breaking change?
Further comments