No PRs are reviewed if GitHub API responses from Cache

[dpordomingo]

When GitHub API answer from cache provider/github/watcher.go#L363

resp.Header.Get("X-From-Cache") == "1"

the project PRs are not listed for review because it is returned a NoErrNotModified error provider/github/watcher.go#L318

so no PR is handled provider/github/watcher.go#L198.

This is a problem if lookout didn't review that refs/pull/%d/head yet, but it is returned from cache by GitHub API. I think it can happen if the DB is dropped between reviews, or if the last review failed.

[dpordomingo]

I could not reproduce it always; last time that I could, it was starting lookout for 3 times in a row in a very short period of time.

Once the GitHub API starts answering from cache, it takes some (many) time to get fresh new responses. The only way to get new responses without waiting is adding a new commit to the PR (or maybe a manual comment, I didn't try this other way)

I wonder if this issue could be the cause of the behavior seen during the @vmarkovtsev presentation at EngDemo-2019-01-22 when the first PR was not reviewed by lookout, and then he created a new one, and then it was triggered the review of the two PRs: the new and the old one.

[dpordomingo]

Does it make sense to bypass the check of NoErrNotModified provider/github/watcher.go#L198 if there is no successful validation for the returned refs/pull/%d/head

[se7entyse7en]

Sorry guys, but I don't have a complete view of the project yet, so maybe part of the things that I'm going to say maybe already exist or maybe not 😄.

What we store in the DB is one of the part that I don't know. But an idea could be to store the timestamp of the latest update for each pull request. So I guess the watcher that once triggered, the first thing it does is to store the timestamp. I say "the first thing to do" in order to minimize any possibile error that might occur between the request to the GH api and the saving of the timestamp. On the other hand we also store the timestamp of the latest comments posted for each pull request.

Everytime we have a NotModified we could then check for these values and proceed if latestUpdate > latestPost.

[se7entyse7en]

Linking relevant discussion. @dpordomingo do you think that we can remove the help needed label given the provided input by @smola in the discussion?

[se7entyse7en]

I've added the blocked label because we had a discussion on whether we want to add other functionalities to the memory queue on go-queue link. We also had a discussion about alternatives such as bypassing the queue, or ignoring the cache on restart.

[carlosms]

I've realized the following. In the description @dpordomingo says I think it can happen if the DB is dropped between reviews

If the DB is cleared, the described behaviour is expected. Having a zero-capacity queue, or a persistent queue, will not change this.
This is how it works:

1. We get github PR
2. create a lookout review event
3. send the event to the queue, save to the http cache on disk that the github PR is processed
4. get event from queue, process it, save on the DB that the lookout event was processed.

So, if you delete the DB expecting to force a new processing of the PRs, you should also delete the http cache.

I agree that even if this is technically correct, it can be confusing and not expected. I think it's reasonable to use a clear http cache when lookoutd is restarted, for example using ioutil.TempDir.

I would still keep the git library /tmp/lookout persistent between lookoutd restarts, as the contents take more time to download.

...

Now, about the problem of losing the memory queue contents on restart. I think losing jobs from the memory queue is only a problem if the http cache gets out of sync with the queue.

So f the http cache and the memory queue are both on a clean state every time lookoutd is restarted, I think we don't have a need for the unbuffered memory queue done in src-d/go-queue#18.

[se7entyse7en]

I agree that even if this is technically correct, it can be confusing and not expected.

I think that it shouldn't be that unexpected. I mean that if you're going to manually drop the DB you should expect that something unexpected could happen. If it was done with the purpose of starting lookout from a clean state, we could add a clean command that stops lookout and then drops both the DB and deletes the http cache. Then if the user uses the clean command I agree that it is unexpected that some PRs are not being processed after restart.

I think it's reasonable to use a clear http cache when lookoutd is restarted.

Yup, I like this solution too as it should be very simple.

I think losing jobs from the memory queue is only a problem if the http cache gets out of sync with the queue.

What do you mean exactly with out of sync exactly? Do you mean having unacknowledged jobs in the queue, but the corresponding url already validated in the cache? If we clean the cache on restart this shouldn't be a problem anymore, we're just going to process some events twice, isn't it?

I think we don't have a need for the unbuffered memory queue done in src-d/go-queue#18.

The only extra thing that the unbuffered queue provides compared to cleaning the cache is that it may avoid calling Github's api for some urls, but if this is something that we can actually afford I think that is good tradeoff to just clean the cache as @smacker initially pointed out.

It would be interesting to have a metric that counts the median number of unacknowledged jobs in the queue.

[carlosms]

I think losing jobs from the memory queue is only a problem if the http cache gets out of sync with the queue.

What do you mean exactly with out of sync exactly? Do you mean having unacknowledged jobs in the queue, but the corresponding url already validated in the cache? If we clean the cache on restart this shouldn't be a problem anymore, we're just going to process some events twice, isn't it?

Sorry if it was not clear, you have it right. What I meant is the problem in master happens because the memory queue and the http cache get out of sync. But if we clear the http cache on restart, the problem disappears as you say.

I think we don't have a need for the unbuffered memory queue done in src-d/go-queue#18.

The only extra thing that the unbuffered queue provides compared to cleaning the cache is that it may avoid calling Github's api for some urls, but if this is something that we can actually afford I think that is good tradeoff to just clean the cache as @smacker initially pointed out.

Yeah, I think the worst case scenario is actually equivalent to having an expired cache, or a cache invalidated because GitHub has new PRs. So we are forcing a new call to github that will happen anyway sooner or later. So, no real downside.

[se7entyse7en]

So just to recap we can remove the blocked label and solve this by cleaning the cache on restart, right?