-
Notifications
You must be signed in to change notification settings - Fork 248
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
Request hedging #187
Request hedging #187
Conversation
internal/datastore/proxy/hedging.go
Outdated
var digestLock sync.Mutex | ||
|
||
digests := []*tdigest.TDigest{ | ||
tdigest.NewWithCompression(1000), | ||
tdigest.NewWithCompression(1000), | ||
} |
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 wonder if it's overkill to pull this out into its own type that has thread-safe methods like Quantile()
.
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'm honestly not sure, we would still need a lock around any mutations to the slice, and we're reading from the digests just as often as we are writing to them.
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.
But if we move the writes into their own routine (see below), we could in theory either switch to a RWLock or, alternatively, have the "write" call also store the current first quantile value, and have reads just use whatever is stored?
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.
Is there a change to make here?
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'd see if we could avoid the lock entirely on the read, only take it on the write (ideally) and do so asynchronously from the critical path
10303bf
to
bb6c13d
Compare
bb6c13d
to
512be9d
Compare
internal/datastore/proxy/hedging.go
Outdated
|
||
return hedgingProxy{ | ||
delegate, | ||
newHedger(timeSource, initialSlowRequestThreshold, maxSampleCount, hedgingQuantile, 1000), |
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.
Place the compression into a const?
go req(ctx, responseReady) | ||
|
||
select { | ||
case <-responseReady: |
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.
Maybe put a comment here? I know Go doesn't auto-fallthrough, but it first read that way
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.
This is a select
not a switch...
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 know, but it still looks that way at first read through
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.
idk about that and even for most cases you're unlikely to be even want to use fallthrough because you can do multiple cases separated by commas (e.g. case This == false, That == false:
).
internal/datastore/proxy/hedging.go
Outdated
<-responseReady | ||
} | ||
|
||
// Compute how long it took for us to get any answer |
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.
Does order matter at all here? If not, I'd say we update the digest in a goroutine to prevent this from being on the critical call path
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.
They take microseconds to query and update according to the original author of the algorithm.
internal/datastore/proxy/hedging.go
Outdated
var digestLock sync.Mutex | ||
|
||
digests := []*tdigest.TDigest{ | ||
tdigest.NewWithCompression(1000), | ||
tdigest.NewWithCompression(1000), | ||
} |
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.
But if we move the writes into their own routine (see below), we could in theory either switch to a RWLock or, alternatively, have the "write" call also store the current first quantile value, and have reads just use whatever is stored?
Signed-off-by: Jake Moshenko <jacob.moshenko@gmail.com>
Signed-off-by: Jake Moshenko <jacob.moshenko@gmail.com>
512be9d
to
8f0e340
Compare
internal/datastore/proxy/hedging.go
Outdated
"github.com/authzed/spicedb/internal/datastore" | ||
) | ||
|
||
var hedgeableCount = prometheus.NewCounter(prometheus.CounterOpts{ |
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.
you should just use promauto
Signed-off-by: Jake Moshenko <jacob.moshenko@gmail.com>
Signed-off-by: Jake Moshenko <jacob.moshenko@gmail.com>
Signed-off-by: Jake Moshenko <jacob.moshenko@gmail.com>
60e4f5d
to
3250215
Compare
From the paper:
Fixes #19