Skip to content
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

Graceful handing of corrupted cache index #1110

Merged
merged 1 commit into from
Jun 24, 2016
Merged

Graceful handing of corrupted cache index #1110

merged 1 commit into from
Jun 24, 2016

Conversation

jgosmann
Copy link
Collaborator

@jgosmann jgosmann commented Jun 24, 2016

Description:
Gracefully handles a corrupted cache index by clearing the cache an creating a new index.

Motivation and context:
In some instances the cache index can get corrupted (i.e. killing the process while the cache index is being written) and that would give an exception whenever one tries to use the cache. See #1097.

Interactions with other PRs:
Related to #1107 which makes it less likely to end up with a corrupted index.

How has this been tested?
Added a regression test, made sure it fails and fixed the problem which makes the test pass.

Where should a reviewer start?
Either the test or the fix depending on personal preference.

How long should this take to review?

  • Quick (less than 40 lines changed or changes are straightforward)

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have read the CONTRIBUTING.rst document.
  • I have updated the documentation accordingly. (no changes necessary imo)
  • I have included a changelog entry.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@jgosmann jgosmann added this to the 2.1.1 release milestone Jun 24, 2016
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @tbekolay to be a potential reviewer

@tbekolay
Copy link
Member

This looks good me, thanks for prepping this quickly! I added a commit to fix the flake8 error, and add some assert statements. The asserts aren't strictly necessary but feels like something should be asserted (I could be persuaded to leave them out).

This probably can use a changelog entry, which I could write in the merge. Can someone else look at this quickly, as it's the last thing for 2.1.1?

@hunse
Copy link
Collaborator

hunse commented Jun 24, 2016

Yeah, LGTM.

@jgosmann
Copy link
Collaborator Author

additional commit LGTM, do you want to also include #1107 in 2.1.1?

@hunse hunse mentioned this pull request Jun 24, 2016
5 tasks
@hunse
Copy link
Collaborator

hunse commented Jun 24, 2016

I would not include #1107, since it still needs a bit more testing I think. Hopefully 2.2.0 will be out in a week or two, so it shouldn't be too long of a wait.

@jgosmann
Copy link
Collaborator Author

This probably can use a changelog entry, which I could write in the merge.

ok, leaving this to you. Didn't do it before because I first need the PR number and after I opened the PR I went for lunch before I was able to write the changelog.

@tbekolay tbekolay removed their assignment Jun 24, 2016
@tbekolay
Copy link
Member

Fair ;) Will do the merge now, then the release.

@tbekolay
Copy link
Member

... after my lunch, that is.

@tbekolay tbekolay merged commit da87e14 into master Jun 24, 2016
@tbekolay tbekolay deleted the fix-1097 branch June 24, 2016 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants