Taras Bobrovytsky has posted comments on this change. Change subject: Add Breakpad minidump collection script ......................................................................
Patch Set 3: (18 comments) http://gerrit.cloudera.org:8080/#/c/2997/3/bin/collect_minidumps.py File bin/collect_minidumps.py: Line 29: gz > nit: gzip Done Line 36: self > Can you format those into a single line? Except if we follow a different co I think it's ok to keep it as is, I think it's very readable, also look at similar formatting here in our existing code: http://github.mtv.cloudera.com/CDH/Impala/blob/cdh5-trunk/tests/comparison/discrepancy_searcher.py#L687 Line 48: resulting_sizes > explain in the comment that it maps the number of files to the size. Done Line 90: tarballed > collected? Done Line 93: min_num > can you initialize one variable per line, as it's easier to read? Done Line 95: / > you can make this // as it's integer division. I don't even see why it work In Python 2.x, / is integer division. However, in Python 3.x / could return floats. I guess // is more defensive, in case we ever switch to Python 3.x. Done. Line 103: Makes > nit: Make... Return... Done Line 135: role_type > isn't that the role? Or the role name? Type sounds like something different I agree, I think it should be called role_name. Done. I got role_type from the document (where I now made a comment suggesting to change it). Line 158: result > You still need to append the role name to the path, as the flag files will Really? I was under the impression that the path is completely encoded in the each process' flag file. Why would --minidump_path in catalogserver_flags point to a common base dir instead of to the specific dir where catalogd minidumps will actually go? Line 169: 'If the total size exceeds this value, ' : > Make single line? Done Line 181: file_archiver = FileArchiver(source_dir=minidump_dir, > multiple arguments per line. I think it's much more readable the way it is. I think this type of formatting is more or less part of our style. http://gerrit.cloudera.org:8080/#/c/2997/3/bin/generate_minidump_collection_testdata.py File bin/generate_minidump_collection_testdata.py: Line 77: DAEMONS > ROLE_NAMES? Done Line 83: rmtree > wouldn't it be sufficient to just overwrite the files? This looks like some Done Line 92: timestamp > It this mtime or ctime? Can you add a comment? Done Line 96: 8192 > add comments why you chose those values I actually don't have a good explanation why I chose those numbers. Maybe they are not necessarily good. Also maybe the overall algorithm for generating the files is not that great either. (Also, not sure if it's worth spending the time improving it). Line 112: 256 > comment on magic numbers Same as above, I chose them by playing around with different values. No deep thinking behind them :) Line 114: interval = 0 if options.num_minidumps == 1 else ( : end_timestamp - start_timestamp) / (options.num_minidumps - 1) > can you make this a proper if: else:? I assume it'd be easier to read. Done Line 116: range > xrange? Done -- To view, visit http://gerrit.cloudera.org:8080/2997 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I85b3643133e28eca07507ac2a79acbf73128456f Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Ishaan Joshi <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes
