Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/16881 )
Change subject: IMPALA-9865: part 2/2: add verbosity to profile tool ...................................................................... Patch Set 7: (6 comments) I have 1 follow up question and 1 new finding. http://gerrit.cloudera.org:8080/#/c/16881/6/be/src/util/impala-profile-tool.cc File be/src/util/impala-profile-tool.cc: http://gerrit.cloudera.org:8080/#/c/16881/6/be/src/util/impala-profile-tool.cc@88 PS6, Line 88: RuntimeProfileBase::Verbosity configured_verbosity = > We shouldn't actually be using the default value, since we only use if it's Done http://gerrit.cloudera.org:8080/#/c/16881/6/be/src/util/impala-profile-tool.cc@136 PS6, Line 136: if (FLAGS_profile_verbosity == "") { > Yes and no. We can set the verbosity level to any level for any profile, bu Make sense. Thanks for clarifying this. http://gerrit.cloudera.org:8080/#/c/16881/7/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/16881/7/be/src/util/runtime-profile.cc@2723 PS7, Line 2723: if (verbosity <= Verbosity::LEGACY) continue; Looks like this condition can be flipped and merged with the branch bellow? if (verbosity >= Verbosity::DEFAULT && v.second.second.size() > 1) { http://gerrit.cloudera.org:8080/#/c/16881/6/bin/rat_exclude_files.txt File bin/rat_exclude_files.txt: http://gerrit.cloudera.org:8080/#/c/16881/6/bin/rat_exclude_files.txt@157 PS6, Line 157: testdata/impala-profiles/impala_profile_log_tpcds_compute_stats.expected.txt > Done Resolved http://gerrit.cloudera.org:8080/#/c/16881/6/tests/observability/test_profile_tool.py File tests/observability/test_profile_tool.py: http://gerrit.cloudera.org:8080/#/c/16881/6/tests/observability/test_profile_tool.py@36 PS6, Line 36: self._compare_profile_tool_output([], > Leaving --profile_verbosity unset is different from setting it to --legacy Ok, I understand the difference now. Setting --profile_verbosity=legacy means forcing legacy format, regardless of the profile_version. Since we don't test with --profile_verbosity=legacy, am I correct to assume that accurate backward-compatible parsing of v2 profile to v1 representation is not the goal of impala-profile-tool? http://gerrit.cloudera.org:8080/#/c/16881/6/tests/observability/test_profile_tool.py@38 PS6, Line 38: get_profile_path('impala_profile_log_tpcds_compute_stats.expected.txt')) > See above comment - this is actually kinda different from legacy Got it, thanks! -- To view, visit http://gerrit.cloudera.org:8080/16881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82618a813e29af7996dfaed78873b2a73bc0231d Gerrit-Change-Number: 16881 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 22 Dec 2020 03:12:09 +0000 Gerrit-HasComments: Yes
