Lars Volker has posted comments on this change. Change subject: Add Breakpad minidump collection script ......................................................................
Patch Set 3: (17 comments) http://gerrit.cloudera.org:8080/#/c/2997/3/bin/collect_minidumps.py File bin/collect_minidumps.py: Line 29: gz nit: gzip Line 36: self Can you format those into a single line? Except if we follow a different convention in python, then it'd be fine of course. Line 48: resulting_sizes explain in the comment that it maps the number of files to the size. Line 90: tarballed collected? Line 93: min_num can you initialize one variable per line, as it's easier to read? Line 95: / you can make this // as it's integer division. I don't even see why it works like this, as you'll get a float, which ends up in xrange(). Line 103: Makes nit: Make... Return... Line 135: role_type isn't that the role? Or the role name? Type sounds like something different. Line 169: 'If the total size exceeds this value, ' : Make single line? Line 181: file_archiver = FileArchiver(source_dir=minidump_dir, multiple arguments per line. 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? Line 83: rmtree wouldn't it be sufficient to just overwrite the files? This looks like someone might specify /tmp to have the files generated there and get bitten by it. Line 92: timestamp It this mtime or ctime? Can you add a comment? Line 96: 8192 add comments why you chose those values Line 112: 256 comment on magic numbers 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. Line 116: range xrange? -- 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
