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 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 ? 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 not seem to override it back to Verbosity::LEGACY. Does this implies that both v1 and v2 profile input can be parsed into any verbosity level? 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? 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 argument `--profile_verbosity=legacy` ? Should we also test between v1 vs v2 profile input when --profile_verbosity is not specified? 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.expected" for clarity? -- 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 <tarmstr...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 21 Dec 2020 19:18:57 +0000 Gerrit-HasComments: Yes
