Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24434003:
CQ: add landmines to delete database after unclean shutdown (Closed)
|
Created:
7 years, 3 months ago by Paweł Hajdan Jr. Modified:
7 years, 1 month ago CC:
chromium-reviews, cmp-cc_chromium.org Visibility:
Public. |
DescriptionCQ: add landmines to delete database after unclean shutdown
This is an improved version of https://codereview.chromium.org/23011039/
using an approach recommended by Isaac.
I've tested it locally (thanks to recently committed changes),
and it behaves as expected and doesn't break any other things.
BUG=239272
R=iannucci@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225276
Patch Set 1 #
Total comments: 23
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #Messages
Total messages: 7 (0 generated)
Please review. Robbie: primary reviewer Isaac: mostly FYI, and review latency is likely to be higher, but I'm going to apply your review comments even if this is committed before such comments; please indicate clearly whether you consider the comments blocking the CL (review latency)
makes a lot of sense, generally looks good to me https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py File commit-queue/commit_queue.py (right): https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:127: """Exception that indicates being interrupted by a caught signal.""" It might be nice to eventually encapsulate the actual signal type which we caught, but I think right now it's only SIGINT. https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:253: os.remove(db_path) Can we save the db off to a mktemp backup instead, and then note that in the log message? Like db.crashed.Wm3fks ? https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:258: os.remove(db_path) same here with backup/log message https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:261: with open(landmine_path, 'w') as _landmine_file: you can just leave off the 'as ...' part. https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:337: except SignalInterrupt as e: unless you want to do something with e, you should just except SignalInterrupt: https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:343: print 'By bye' 'Bye bye: caught SignalInterrupt' Also, should these prints be logs instead? https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:346: except KeyboardInterrupt as e: same here re: 'as e' https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:352: print 'Bye bye' maybe 'Unclean shutdown! caught KeyboardInterrupt' when I did the keyboard interrupt stuff, I wanted to put interrupt points in more places so that the cq would always terminate basically immediately, but M-A said this would be very hard to do right so I never bothered :/ https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:356: parser.error(str(e)) Can we log here? https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:361: os.remove(landmine_path) Maybe add a log/print here
Makes sense. Looks good to me % Robbie's comments.
Thanks for review. PTAL. https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py File commit-queue/commit_queue.py (right): https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:127: """Exception that indicates being interrupted by a caught signal.""" On 2013/09/25 19:47:07, iannucci wrote: > It might be nice to eventually encapsulate the actual signal type which we > caught, but I think right now it's only SIGINT. Done. https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:253: os.remove(db_path) On 2013/09/25 19:47:07, iannucci wrote: > Can we save the db off to a mktemp backup instead, and then note that in the log > message? Like db.crashed.Wm3fks ? Done. https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:258: os.remove(db_path) On 2013/09/25 19:47:07, iannucci wrote: > same here with backup/log message Done. https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:261: with open(landmine_path, 'w') as _landmine_file: On 2013/09/25 19:47:07, iannucci wrote: > you can just leave off the 'as ...' part. Done. https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:337: except SignalInterrupt as e: On 2013/09/25 19:47:07, iannucci wrote: > unless you want to do something with e, you should just > > except SignalInterrupt: Done. https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:343: print 'By bye' On 2013/09/25 19:47:07, iannucci wrote: > Also, should these prints be logs instead? Not sure, they were prints before so I left it that way. There are potentially many more things to be fixed with CQ while I'm there. https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:346: except KeyboardInterrupt as e: On 2013/09/25 19:47:07, iannucci wrote: > same here re: 'as e' Done. https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:352: print 'Bye bye' On 2013/09/25 19:47:07, iannucci wrote: > maybe 'Unclean shutdown! caught KeyboardInterrupt' > > when I did the keyboard interrupt stuff, I wanted to put interrupt points in > more places so that the cq would always terminate basically immediately, but M-A > said this would be very hard to do right so I never bothered :/ Done. https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:356: parser.error(str(e)) On 2013/09/25 19:47:07, iannucci wrote: > Can we log here? I'd prefer not to clutter the patch with random cleanups around it. https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:361: os.remove(landmine_path) On 2013/09/25 19:47:07, iannucci wrote: > Maybe add a log/print here I don't think it's needed - if you can come up with a reasonable reason to log I can add it - other than it _might_ be useful.
lgtm % spelling nit :) https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py File commit-queue/commit_queue.py (right): https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:356: parser.error(str(e)) On 2013/09/25 21:53:36, Paweł Hajdan Jr. wrote: > On 2013/09/25 19:47:07, iannucci wrote: > > Can we log here? > > I'd prefer not to clutter the patch with random cleanups around it. Actually I think parser.error will log/print anyhow https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:361: os.remove(landmine_path) On 2013/09/25 21:53:36, Paweł Hajdan Jr. wrote: > On 2013/09/25 19:47:07, iannucci wrote: > > Maybe add a log/print here > > I don't think it's needed - if you can come up with a reasonable reason to log I > can add it - other than it _might_ be useful. Just reacting to the comment. If we never expect to get here, I would be more inclined to assert False. https://codereview.chromium.org/24434003/diff/6001/commit-queue/commit_queue.py File commit-queue/commit_queue.py (right): https://codereview.chromium.org/24434003/diff/6001/commit-queue/commit_queue.... commit-queue/commit_queue.py:143: tmp_file.write(db_file.read()) use one of the shutil copy methods to avoid reading the whole file into memory, maybe? https://codereview.chromium.org/24434003/diff/6001/commit-queue/commit_queue.... commit-queue/commit_queue.py:363: print 'By bye (SignalInterrupt)' 'Bye bye' Bye -> abbreviation of 'goodbye' By -> preposition identifying the action of an agent 'This was done by Sam.' :D
https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py File commit-queue/commit_queue.py (right): https://codereview.chromium.org/24434003/diff/1/commit-queue/commit_queue.py#... commit-queue/commit_queue.py:361: os.remove(landmine_path) On 2013/09/25 22:15:48, iannucci wrote: > On 2013/09/25 21:53:36, Paweł Hajdan Jr. wrote: > > On 2013/09/25 19:47:07, iannucci wrote: > > > Maybe add a log/print here > > > > I don't think it's needed - if you can come up with a reasonable reason to log > I > > can add it - other than it _might_ be useful. > > Just reacting to the comment. If we never expect to get here, I would be more > inclined to assert False. Added a TODO. It seems too risky to change behavior beyond scope of this patch. https://codereview.chromium.org/24434003/diff/6001/commit-queue/commit_queue.py File commit-queue/commit_queue.py (right): https://codereview.chromium.org/24434003/diff/6001/commit-queue/commit_queue.... commit-queue/commit_queue.py:143: tmp_file.write(db_file.read()) On 2013/09/25 22:15:49, iannucci wrote: > use one of the shutil copy methods to avoid reading the whole file into memory, > maybe? Done. https://codereview.chromium.org/24434003/diff/6001/commit-queue/commit_queue.... commit-queue/commit_queue.py:363: print 'By bye (SignalInterrupt)' On 2013/09/25 22:15:49, iannucci wrote: > 'Bye bye' > > Bye -> abbreviation of 'goodbye' > By -> preposition identifying the action of an agent 'This was done by Sam.' > > :D Done. Actually this was accidental deletion in vim, not a language mistake.
Message was sent while issue was closed.
Committed patchset #3 manually as r225276 (presubmit successful). |
