impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Taras Bobrovytsky (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) Add Breakpad minidump collection script
Date Wed, 11 May 2016 02:03:05 GMT
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 <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Ishaan Joshi <ishaan@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: ankur.014@gmail.com
Gerrit-HasComments: Yes

Mime
View raw message