Skip to content
This repository has been archived by the owner on Feb 4, 2020. It is now read-only.

Fix crash when a manifest was evicted but the object is in the cache #237

Merged
merged 2 commits into from
Oct 31, 2016

Conversation

siu
Copy link
Contributor

@siu siu commented Oct 26, 2016

Add tests that check if the evicted objects and manifests are handled properly, attempt to reproduce #155.

@siu
Copy link
Contributor Author

siu commented Oct 26, 2016

Finally, it is crashing!

@frerich
Copy link
Owner

frerich commented Oct 26, 2016

Indeed, removing a manifest will not remove all referenced artifacts (because those artifacts are still usable if you don't use the 'direct' mode at all). So it may be that when adding an entry in direct mode, a manifest is missing but the referenced artifact is already there.

I'm curious to see whether the patch I mentioned in #236 (comment) makes the test pass.

@codecov-io
Copy link

codecov-io commented Oct 26, 2016

Current coverage is 88.86% (diff: 100%)

Merging #237 into master will not change coverage

@@             master       #237   diff @@
==========================================
  Files             1          1          
  Lines           997        997          
  Methods           0          0          
  Messages          0          0          
  Branches        158        158          
==========================================
  Hits            886        886          
  Misses           83         83          
  Partials         28         28          

Powered by Codecov. Last update c852c14...a396f2b

@siu siu changed the title Add tests checking behaviour after objects and manifests are evicted Fix crash when a manifest was evicted but the object is in the cache Oct 26, 2016
@siu
Copy link
Contributor Author

siu commented Oct 26, 2016

Yes, it fixes it, all tests are passing.

@frerich
Copy link
Owner

frerich commented Oct 26, 2016

Great job, I think this resolves #155 after all!

@siu
Copy link
Contributor Author

siu commented Oct 31, 2016

@frerich I think you closed the issue instead of merging it.

@frerich frerich reopened this Oct 31, 2016
@frerich frerich merged commit 718c651 into frerich:master Oct 31, 2016
@frerich
Copy link
Owner

frerich commented Oct 31, 2016

Ooops @siu, sorry! I seem to have hit the wrong button - that was not intentional. Thanks for catching!

@siu siu deleted the evictedManifestsAndObjectsTest branch November 7, 2016 11:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants