Pull request list apis with cache #118
Conversation
shana
force-pushed
the
feature/pr/streaming-cache
branch
from
316ad6d
to
18f73b3
Compare
shana
changed the title
CacheIndex stores a list of string keys of all the objects that are a part of a query - all pull requests for a repository (regardless of state); all repositories for a user, etc. When a query is performed (GetPullRequests for instance), the CacheIndex object is obtained from akavache and all the objects referenced by the index keys are returned. If it needs to be refreshed, the fetch function is called and each returned object will be added/refreshed in akavache and the index will also be updated. This way, no matter what type of filtering is performed on the query, only one object of each type will be saved (keys are in the form `user|repo|objecttype|id`), and we can leverage the fact that GitHub list apis return full objects and not trimmed-down indexes, avoiding doing the same query twice (once for list, once for detail), since the object will be fully cached during listing and immediately available for detail. We will want to use sql queries directly at some point, for searches and other more advanced uses, but for now this works. We can use these index objects to create real sql indexes later on.
ModelService and ApiClient support for getting pull requests, and PullRequestModel implementation
shana
force-pushed
the
feature/pr/streaming-cache
branch
from
18f73b3
to
58c5da1
Compare
shana
changed the title
No sense returning TrackingCollection from inside model service, that should be done higher in the stack.
shana
force-pushed
the
feature/pr/streaming-cache
branch
from
e2b7160
to
963c77a
Compare
shana
force-pushed
the
feature/pr/streaming-cache
branch
from
a0d5ac2
to
9fd9edf
Compare
This is a hold-over from the old way we did 2fa handling.
The type of stuff that make for annoying code reviews. :stuck_out_tongue: - Sort and order usings. - Remove redundant qualifiers and tokens. - Use conditional expressions
I missed this one.
| @@ -34,7 +38,7 @@ public static class AkavacheExtensions | |||
| { | |||
| return Observable.Defer(() => | |||
| { | |||
| var absoluteExpiration = blobCache.Scheduler.Now + maxCacheDuration; | |||
| var absoluteExpiration = blobCache.Scheduler.Now.ToUniversalTime() + maxCacheDuration; | |||
There was a problem hiding this comment.
Given that Scheduler.Now returns a DateTimeOffset, the call to ToUniversalTime is unnecessary. The value returned represents the same point in time as the original value. Here's some code to demonstrate.
var localTime = new DateTimeOffset(new DateTime(2007, 6, 15,
12, 0, 0), TimeSpan.FromHours(8));
var universalTime = localTime.ToUniversalTime();
Console.WriteLine("Local offset: " + localTime.Offset);
Console.WriteLine("Universal offset: " + universalTime.Offset
);
Console.WriteLine("Difference: " + (localTime - universalTime
));Output
Local offset: 08:00:00
Universal offset: 00:00:00
Difference: 00:00:00
There was a problem hiding this comment.
Ah, yeah, I just replaced all the calls and didn't notice some didn't need it.
There was a problem hiding this comment.
So I poked further, and it's not that Scheduler.Now returns a DateTimeOffset, it's that the implementation actually returns SystemClock.UtcNow, which is why there's no difference between the two. Because the property is called Now and not UtcNow, I thought it returned DateTimeOffset.Now, which is local time and definitely not what we want. Sigh at API inconsistencies...
As long as that doesn't change, it's all good.
|
I'm still reviewing, but I got tired and decided to 💤 |
In equality methods and operators, I prefer not to mix reference and value checks in one line. These are different checks and the value parts may change in the future. These are very easy to get wrong, so I prefer to keep them as readable and split up as possible.
Scheduler.Now returns UtcNow and it would be nice if that were documented.
... when refreshing the index.
|
I figured out how to delete items from the collection when the cache is refreshed from live data, so now whenever the cache is refreshed from the api, if anything is deleted (probably not pull requests, but when we start tracking repositories with this, it's going to happen), whatever items were deleted remotely will be removed from the cache index and from the collection. |
|
This branch is a-:+1: and ready for :eyes:! |
| UpdatedAt = updatedAt ?? CreatedAt; | ||
| } | ||
|
|
||
| public void CopyFrom(IPullRequestModel other) |
There was a problem hiding this comment.
This method is confusing. I was wondering why it doesn't update Number and Author and it occurred to me, this isn't a "copy" method, it's an "update" method, right? That's why it only updates fields that could possibly change for a given pull request.
Should we perhaps ensure that the other.Id matches the current Id?
There was a problem hiding this comment.
Yeah, makes sense for this implementation to check that the id matches. I've pushed a fix for that.
In this case the copy is only copying what it wants updated, yes. In other cases the copy might be a full deep copy, it's for the implementation to decide. I've documented the interface to clarify this.
There was a problem hiding this comment.
Sorry to nitpick, but would CopyPropertiesFrom be a more descriptive name?
There was a problem hiding this comment.
Well, you can technically copy fields too, if you want 😄 It depends on the actual implementation, it could grab values via methods to copy, too.
There was a problem hiding this comment.
True, but this particular implementation only copies properties. 😄
There was a problem hiding this comment.
Well yes, but changing the name of the method in the interface because this implementation happens to only copy properties doesn't really feel logical. 😜
Also used a method group.
|
🍐 back to you! There's no visible change to the UI in this PR, right? Still laying groundwork? |
|
Yeah, no UI here yet, this is the last groundwork PR! After this, we'll have all the pieces in place, just add paint! |
| public void CopyFrom(IPullRequestModel other) | ||
| { | ||
| if (Number != other.Number) | ||
| throw new ArgumentException("Instance to copy from must match", nameof(other)); |
There was a problem hiding this comment.
Probably would be useful to print out what the current ID is and what the other one was.
Pull request list apis with cache
|
✨ |
This PR depends on #111
This PR adds a cache index for akavache objects, and implements getting lists of pull requests from github into the cache and index and streaming them out via TrackingCollection.
CacheIndex stores a list of string keys of all the objects that are a part of a query - all pull requests for a repository (regardless of state); all repositories for a user, etc. When a query is performed (GetPullRequests for instance), the CacheIndex object is obtained from akavache and all the objects referenced by the index keys are returned. If it needs to be refreshed, the fetch function is called and each returned object will be added/refreshed in akavache and the index will also be updated.
This way, no matter what type of filtering is performed on the query, only one object of each type will be saved (keys are in the form
user|repo|objecttype|id), and we can leverage the fact that GitHub list apis return full objects and not trimmed-down indexes, avoiding doing the same query twice (once for list, once for detail), since the object will be fully cached during listing and immediately available for detail.We will want to use sql queries directly at some point, for searches and other more advanced uses, but for now this works. We can use these index objects to create real sql indexes later on.