search

graphite-project / carbonate

Utilities for managing graphite clusters
MIT License
516 stars 80 forks source link

Make the heal process resilient #42

Closed jjneely closed 8 years ago

jjneely commented 9 years ago
jssjr commented 9 years ago

Thanks for the PR and apologies for the delayed response!

Do not overwrite the target WSP file if the source file is corrupt.

This seems like the ideal behavior. Good catch!

Make a backup of the source file if we replace it

I believe you meant target file in this statement. I'm torn about this because it has the ability to clutter your storage directory with backups of corrupt (and likely useless) files. I'm not sure how useful it is to keep the corrupt whisper files if the user's intent is to overwrite them.

Would it be simpler to expose a flag to the user that defines how to handle corrupt targets? Something like --overwrite-corrupt. This would need to default to true to keep backwards compatible behavior and I don't love the name, but it seems like having a flag would make the corrupt handling behavior less surprising to the user.

Handle any other possible exception that can be generated by fill_archives(), log,

I don't like this at all. While catching Exception is better than a bare except, I'd like to identify what exception we want to gracefully handle and be explicit about that behavior. In my opinion, if something unexpected happens I think the script should fail hard.

jssjr commented 9 years ago

FWIW, I love the intent here. I just want to make sure we get things right before merging.

jjneely commented 9 years ago

It sounds like I should separate the three improvements into their own PRs. That will probably help make progress here.

jjneely commented 9 years ago

See PR #52

deniszh commented 8 years ago

Lint is still complaining

carbonate/sync.py:76:70: E502 the backslash is redundant between brackets
carbonate/sync.py:88:80: E501 line too long (82 > 79 characters)

@jjneely - could you please fix that, if possible?

deniszh commented 8 years ago

Replaced with https://github.com/graphite-project/carbonate/pull/69