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

Conversation

@siu
Copy link
Contributor

@siu siu commented Oct 16, 2017

The signal handlers were introduced to avoid cache corruption when the
clcache process is stopped in the middle of a write to the cache
(statistics, manifests or objects). See #233. Even if SIGINT and SIGTERM
were ignored the cache still had a chance to be corrupted in the event
of a SIGTERM (which cannot be ignored).

Since #233 the writing of files to the cache has been improved to
replace the files atomically, reducing the risk of storing corrupted
files in the cache. See pull requests #286, #292 and #296. Therefore
ignoring these signals is not needed anymore.

@siu
Copy link
Contributor Author

siu commented Oct 16, 2017

@frerich I think it is safe to do this now but I don't know if it has any side effects when the clcachesrv or memcache is used.

@frerich
Copy link
Owner

frerich commented Oct 16, 2017

I think I agree that removing the signal handlers should be safe now, but I'll have to think about this some more (and remind myself of the discussions we had back when the handlers were introduced). Thanks for bringing this topic up and providing a patch!

@siu
Copy link
Contributor Author

siu commented Oct 16, 2017

Should close #263, #261.

@frerich
Copy link
Owner

frerich commented Oct 29, 2017

@siu Can you rebase your PR on current master? Thanks! I think that otherwise, this is safe to merge (after dropping Python 3.3 support).

@siu siu force-pushed the removeSignalHandlers branch from 196d68f to 2714cc0 Compare October 30, 2017 13:55
@siu
Copy link
Contributor Author

siu commented Oct 30, 2017

Done, I have a question though, how does one run the tests locally after the refactor in modules? If I run py.test I get this error:

ImportError while importing test module 'C:\src\clcache\tests\test_integration.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
tests\test_integration.py:23: in <module>
    from clcache import __main__ as clcache
E   ImportError: No module named 'clcache'

The signal handlers were introduced to avoid cache corruption when the
clcache process is stopped in the middle of a write to the cache
(statistics, manifests or objects). See frerich#233. Even if SIGINT and SIGTERM
were ignored the cache still had a chance to be corrupted in the event
of a SIGTERM (which cannot be ignored).

Since frerich#233 the writing of files to the cache has been improved to
replace the files atomically, reducing the risk of storing corrupted
files in the cache. See pull requests frerich#286, frerich#292 and frerich#296. Therefore
ignoring these signals is not needed anymore.
@siu siu force-pushed the removeSignalHandlers branch from 2714cc0 to c9e1e3d Compare October 30, 2017 14:00
@ghost
Copy link

ghost commented Nov 1, 2017

@siu Just run pip install -e . before running the tests.

@siu
Copy link
Contributor Author

siu commented Nov 2, 2017

@xoviat, thank you, I will give it a try

@siu
Copy link
Contributor Author

siu commented Nov 2, 2017

@xoviat that worked. Does that mean that if I forget to run pip install -e . I will be running the tests against the clcache installed in the system?

@ghost
Copy link

ghost commented Nov 2, 2017

pip install -e . installs your development checkout of clcache. That means if you edit any files, the changes will take immediate effect.

@siu
Copy link
Contributor Author

siu commented Nov 3, 2017

Cool, that makes a lot of sense, thanks for clarifying the workflow.

@siu
Copy link
Contributor Author

siu commented Nov 6, 2017

@frerich in my opinion this PR is ready, let me know if anything else is blocking the merge.

@frerich
Copy link
Owner

frerich commented Nov 6, 2017

Looks good - I suppose this closes #261.

@frerich frerich merged commit eac41dc into frerich:master Nov 6, 2017
@siu
Copy link
Contributor Author

siu commented Nov 6, 2017

#263 can be closed too.

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.

3 participants