-
Notifications
You must be signed in to change notification settings - Fork 286
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
Add caching to Lookup dispatcher #217
Conversation
c67714b
to
23c5b52
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.
I think we should discuss again why you think check and lookup are different w.r.t depth.
internal/dispatch/caching/caching.go
Outdated
checkTotalCounter prometheus.Counter | ||
checkFromCacheCounter prometheus.Counter | ||
checkTotalCounter prometheus.Counter | ||
|
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.
remove empty line or group properly
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.
Whoops
internal/dispatch/caching/caching.go
Outdated
|
||
var ( | ||
checkResultEntryCost = int64(unsafe.Sizeof(checkResultEntry{})) | ||
lookupResultEntryCost = int64(unsafe.Sizeof(lookupResultEntry{})) |
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 doesn't work because the response objects can contain a variable amount of data. We'll have to compute the costs dynamically per response.
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.
Good point. I'll change it
internal/dispatch/caching/caching.go
Outdated
@@ -158,6 +212,10 @@ func (cd *cachingDispatcher) Close() error { | |||
return nil | |||
} | |||
|
|||
func requestToKey(req *v1.DispatchCheckRequest) string { | |||
func checkRequestToKey(req *v1.DispatchCheckRequest) string { | |||
return fmt.Sprintf("%s@%s@%s", tuple.StringONR(req.ObjectAndRelation), tuple.StringONR(req.Subject), req.Metadata.AtRevision) |
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.
Feel free to throw check//
or some such in this one too
internal/dispatch/caching/caching.go
Outdated
@@ -146,7 +178,29 @@ func (cd *cachingDispatcher) DispatchExpand(ctx context.Context, req *v1.Dispatc | |||
|
|||
// DispatchLookup implements dispatch.Lookup interface and does not do any caching yet. | |||
func (cd *cachingDispatcher) DispatchLookup(ctx context.Context, req *v1.DispatchLookupRequest) (*v1.DispatchLookupResponse, error) { | |||
return cd.d.DispatchLookup(ctx, req) | |||
cd.lookupTotalCounter.Inc() | |||
if req.Metadata.DepthRemaining > 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.
This conditional seems wrong. If it's < 1 shouldn't the whole thing just error immediately?
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.
Yes, but I didn't want to duplicate the error raising code, so I just have it go to dispatch, which will raise the error
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 should be changed now to just let this flow through to the required depth budget > depth remaining the way check works.
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.
Check should also be updated.
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.
What the hell... I changed this... I think my VSCode didn't save my changes to this file :(
Also adds cached dispatcher tests to the overall consistency test suite, and an more complicated lookup example
23c5b52
to
c9e1e09
Compare
Added the |
internal/graph/check.go
Outdated
@@ -362,6 +374,7 @@ func checkResultError(err error, numRequests uint32) CheckResult { | |||
&v1.DispatchCheckResponse{ | |||
Metadata: &v1.ResponseMeta{ | |||
DispatchCount: numRequests, | |||
DepthRequired: 1, |
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.
Don't we want to actually thread the max depth required through here too?
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.
For expand and lookup as well....
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 went back and forth, but I was thinking for errors that it made sense to just always return them, rather than caring about the depth required for them in the cache.
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 think the idea is that the DepthRequired should stand on its own in a CheckResponse
. If it's there, it should be accurate.
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.
Okay, I can change it.
internal/dispatch/caching/caching.go
Outdated
var checkResultEntryCost = int64(unsafe.Sizeof(checkResultEntry{})) | ||
type lookupResultEntry struct { | ||
result *v1.DispatchLookupResponse | ||
computedWithDepthRemaining uint32 |
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 should be changed to storing the required depth budget.
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.
Yeah, I lost this change too. Re-fixed
internal/dispatch/caching/caching.go
Outdated
@@ -146,7 +178,29 @@ func (cd *cachingDispatcher) DispatchExpand(ctx context.Context, req *v1.Dispatc | |||
|
|||
// DispatchLookup implements dispatch.Lookup interface and does not do any caching yet. | |||
func (cd *cachingDispatcher) DispatchLookup(ctx context.Context, req *v1.DispatchLookupRequest) (*v1.DispatchLookupResponse, error) { | |||
return cd.d.DispatchLookup(ctx, req) | |||
cd.lookupTotalCounter.Inc() | |||
if req.Metadata.DepthRemaining > 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.
This should be changed now to just let this flow through to the required depth budget > depth remaining the way check works.
internal/dispatch/caching/caching.go
Outdated
@@ -146,7 +178,29 @@ func (cd *cachingDispatcher) DispatchExpand(ctx context.Context, req *v1.Dispatc | |||
|
|||
// DispatchLookup implements dispatch.Lookup interface and does not do any caching yet. | |||
func (cd *cachingDispatcher) DispatchLookup(ctx context.Context, req *v1.DispatchLookupRequest) (*v1.DispatchLookupResponse, error) { | |||
return cd.d.DispatchLookup(ctx, req) | |||
cd.lookupTotalCounter.Inc() | |||
if req.Metadata.DepthRemaining > 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.
Check should also be updated.
479d56b
to
3e4faff
Compare
3e4faff
to
961cc02
Compare
Updated |
Also adds cached dispatcher tests to the overall consistency test suite, and an more complicated lookup example