Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/11352 )
Change subject: IMPALA-7508: Add Impala Python GDB module ...................................................................... Patch Set 1: (3 comments) Thanks for contributing this. Seems great to share this kind of thing, and also to have examples. I don't have much experience organizing these things, but the inheritance structure seems strange. I'd prefer not having inheritance and just composing your way into the common code. If you do have inheritance, you can avoid multiple-inheritance by having the superclass inherit from gdb.Command. http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py File infra/gdb/impala-gdb.py: http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@22 PS1, Line 22: import gdb Please add a README.md in this directory or include usage here. http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@28 PS1, Line 28: class ImpalaGDB: I think we usually use new-style classes. class ImpalaGDB(object): ... http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@62 PS1, Line 62: class FindFragmentInstances(gdb.Command, ImpalaGDB): : "Find all query fragment instance to thread Id mappings in this impalad." : : def __init__(self): : super(FindFragmentInstances, self).__init__( multiple-inheritance and inheritance in general seem unnecessary here. You could replace the class ImpalaGDB with: instances = get_fragment_instances(gdb) And then use that in your wrapper gdb Commands. -- To view, visit http://gerrit.cloudera.org:8080/11352 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I24e7026f2265954ed592d6f62110cf8cb2c2202a Gerrit-Change-Number: 11352 Gerrit-PatchSet: 1 Gerrit-Owner: Zoram Thanga <zo...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Comment-Date: Wed, 29 Aug 2018 22:16:02 +0000 Gerrit-HasComments: Yes
