Michael Brown has posted comments on this change. Change subject: IMPALA-3489: Add script to extract breakpad symbols from binaries ......................................................................
Patch Set 4: (21 comments) http://gerrit.cloudera.org:8080/#/c/2961/4/bin/dump_breakpad_symbols.py File bin/dump_breakpad_symbols.py: Line 1: #!/usr/bin/env python Is there a reason not to use impala-python instead? Line 17: # in Google breakpad Nit: Breakpad Line 19: # - Scan an impala build dir for ELF files Nit: Impala Line 22: # - Extract an rpm and corresponding debuginfo rpm file This is specific to Impala / CDH RPMs, right? Please clear this up since debuginfo RPMs in general exist outside CDH; they're a common thing. Line 29: # - Goodle breakpad, either installed via the impala toolchain or separately s/Goodle/Google/ capitalization: Breakpad; Impala Line 37: # ./dump_breakpad_symbols -d /tmp/syms -r tmp/impala-2.5.0+cdh5.7.0+0-1.cdh5.7.0.p0.147.el6.x86_64.rpm -s tmp/impala-debuginfo-2.5.0+cdh5.7.0+0-1.cdh5.7.0.p0.147.el6.x86_64.rpm Very long line. Please wrap with \ chars. Line 47: # $IMPALA_TOOLCHAIN/breakpad-*/bin/minidump_stackwalk /tmp/impala-minidumps/impalad/03c0ee26-bfd1-cf3e-43fa49ca-1a6aae25.dmp /tmp/syms Another long line. Line 61: Binary = namedtuple("Binary", "path, debug_path") This is more than just a "Binary", right? It contains a mapping of a binary and its debug info. Could we have a clearer name to convey that? Line 64: def log(msg): : """Log a message to stdout.""" : print msg Do you need this? Why not use the logging module? A lot of our python scripts, and the end-to-end tests as well, use it, so there are plenty of examples. Line 71: sys.stderr.write("ERROR: %s\n" % msg) If you used the logging module, this could be an error-level message. Line 86: dump_syms = glob.glob(os.path.join(toolchain, "breakpad-*", "bin", "dump_syms")) This seems risky. A long-lived working copy of Impala will have a toolchain that accumulates different versions of a provided package. I'm not sure glob is going to be able to give a deterministic result once there is more than one. It's a typical convention for us to expect someone has sourced bin/impala-config.sh before doing anything else. Instead of using glob, why not rely on the environment variable IMPALA_BREAKPAD_VERSION to find the correct breakpad directory? Line 136: def is_file(path): is_regular_file ? Line 167: result = [] Not used. Line 176: binary_base = os.path.join(tmp_dir, 'usr', 'lib', 'impala') : debuginfo_base = os.path.join(tmp_dir, 'usr', 'lib', 'debug', 'usr', 'lib', 'impala') It might be good to make 'usr/lib/impala' and 'usr/lib/debug/usr/lib/impala' constants instead of literals here to the join() function. Line 193: if (args.binary_files): I don't believe the parens are necessary. Line 195: if args.stdin_files: It seems like this chain could all have elif instead. Line 227: with open(tmp_file, 'r') as f: The tmp_fd is never closed before you reopen the same file. Line 230: _, _, _, id, binary = header.split(' ') : out_path = os.path.join(out_dir, binary, id) The use of the 'id' identifier here technically masks the Python builtin function of the same name. Line 235: except: At the very least this should be: except Exception as e: See the exception discussion at https://www.python.org/dev/peps/pep-0008/#programming-recommendations Line 237: os.remove(tmp_file) Catch EnvironmentError here and...I dunno, print an error and move on? Line 238: raise Assuming you make the L235 change... raise e -- To view, visit http://gerrit.cloudera.org:8080/2961 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3ee0972efcb50609407b04cd6f4309b244a84861 Gerrit-PatchSet: 4 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-HasComments: Yes
