Tim Armstrong 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 6: (5 comments) 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; > Is this meant to be initialized as RuntimeProfileBase::Verbosity::LEGACY ? We shouldn't actually be using the default value, since we only use if it's non-empty and parsing succeeds. But clang-tidy was complaining about this anyway so fixing it :) Compiling /home/ubuntu/Impala/be/src/util/impala-profile-tool.cc:134:47: warning: variable 'configured_verbosity' may be uninitialized when used here [clang-diagnostic-conditional-uninitialized] RuntimeProfileBase::Verbosity verbosity = configured_verbosity; ^ /home/ubuntu/Impala/be/src/util/impala-profile-tool.cc:88:3: note: variable 'configured_verbosity' is declared here RuntimeProfileBase::Verbosity configured_verbosity; ^ http://gerrit.cloudera.org:8080/#/c/16881/6/be/src/util/impala-profile-tool.cc@136 PS6, Line 136: verbosity = profile_version < 2 ? RuntimeProfileBase::Verbosity::LEGACY : > In case of v1 profile input and --profile_verbosity=default, this line does Yes and no. We can set the verbosity level to any level for any profile, but it only emits what is present in the thrift profile. Older v1 profiles won't have the TAggregatedRuntimeProfileNode set for the averaged profile, so it will only instantiate a regular RuntimeProfile object when parsing the thrift, which means it will only print the legacy profile information. 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 > Should we add `.txt` suffix for expected output in text format? Done 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([], > Since default verbosity might change in the future, can we add a test using Leaving --profile_verbosity unset is different from setting it to --legacy since it will auto-detect based on the file contents. I think it's a good thing if the test fails when we change the behaviour. I'd like to add some V2 profile tests when I finish up IMPALA-9382 and the format is final. 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')) > Rename expected file name to "impala_profile_log_tpcds_compute_stats_legacy See above comment - this is actually kinda different from legacy -- 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: 6 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 01:15:54 +0000 Gerrit-HasComments: Yes
