Silvius Rus has posted comments on this change. Change subject: IMPALA-3490: Add flag to reduce minidump size ......................................................................
Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/2990/2/be/src/common/global-flags.cc File be/src/common/global-flags.cc: Line 79: DEFINE_int32(minidump_size_limit, 20480, "Size limit for minidump files in KB. If a " Should we include the measurement unit minidump_size_limit_kb? This reads better to me: --minidump_size_limit_kb=20480 versus --minidump_size_limit=20480 Also, calling it minidump_size_limit can be misleading. I can see a support ticket coming in "we told impala to limit minidumps at 10 MB and they are larger". Perhaps something like minidump_size_limit_hint? http://gerrit.cloudera.org:8080/#/c/2990/2/be/src/util/minidump.cc File be/src/util/minidump.cc: Line 193: size_t size_limit = 1024 * FLAGS_minidump_size_limit; Would it be prudent to have a hardcoded hard limit? Also, can 1024 * FLAGS_minidump_size_limit overflow before it gets cast to size_t? http://gerrit.cloudera.org:8080/#/c/2990/2/tests/custom_cluster/test_breakpad.py File tests/custom_cluster/test_breakpad.py: Line 142: def trigger_single_minidump_and_get_size(): Does this method absolutely need to be nested? (I generally find it hard to follow the flow with nested methods.) -- To view, visit http://gerrit.cloudera.org:8080/2990 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2f3aa0df51be9f0bf0755fb288702911cdb88052 Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Silvius Rus <[email protected]> Gerrit-HasComments: Yes
