Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11425 )
Change subject: IMPALA-2063 Remove newline characters in query status. ...................................................................... Patch Set 8: (6 comments) http://gerrit.cloudera.org:8080/#/c/11425/8/be/src/util/runtime-profile.h File be/src/util/runtime-profile.h: http://gerrit.cloudera.org:8080/#/c/11425/8/be/src/util/runtime-profile.h@481 PS8, Line 481: /// Redaction rules are applied on the info string if 'redact' is true. Please add a comment that any trailing whitespace in "value" will be stripped before being inserted. http://gerrit.cloudera.org:8080/#/c/11425/5/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/11425/5/be/src/util/runtime-profile.cc@551 PS5, Line 551: info_strings_.emplace(key, std::move(value)); > The test code I added gets tripped up because there the runtime profile inc I applied the patch and looked at the query profile output at the debug webpage. It appears that stripping all "\n\n" (even those embedded inside the info string) is more than what IMPALA-2063 calls for. For one thing, the "\n\n" embedded inside "value" could be there for legitimate formatting reasons. Stripping them here seems to make the profile potentially less legible or at least break the formatting expected by the creator of "value". I would recommend keeping StripTrailingWhiteSpace() above only. http://gerrit.cloudera.org:8080/#/c/11425/8/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/11425/8/be/src/util/runtime-profile.cc@40 PS8, Line 40: #include "gutil/strings/strip.h" nit: #insert is sorted by alphabetical order http://gerrit.cloudera.org:8080/#/c/11425/8/be/src/util/runtime-profile.cc@541 PS8, Line 541: if (redact) { : Redact(&value); : } nit: can fit in one line http://gerrit.cloudera.org:8080/#/c/11425/8/be/src/util/runtime-profile.cc@546 PS8, Line 546: StripDupCharacters(&value, '\n', 0); Please see comments elsewhere. This change in behavior seems to be more than what IMPALA-2063 calls for. It may actually affect the legibility of the profile in an unexpected way. http://gerrit.cloudera.org:8080/#/c/11425/8/tests/query_test/test_cancellation.py File tests/query_test/test_cancellation.py: http://gerrit.cloudera.org:8080/#/c/11425/8/tests/query_test/test_cancellation.py@188 PS8, Line 188: assert "\n\n" not in profile, \ : "Profile for query %s contains redundant newlines." % handle This assert seems boarder than necessary. I am not sure it's legitimate to enforce that no "\n\n" can never be present in the profile string. For instance, one may add a blank line between various sections of the profile for legibility reasons. So, this assert doesn't seem future proof. -- To view, visit http://gerrit.cloudera.org:8080/11425 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1 Gerrit-Change-Number: 11425 Gerrit-PatchSet: 8 Gerrit-Owner: Michal Ostrowski <mostr...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Michal Ostrowski <mostr...@cloudera.com> Gerrit-Comment-Date: Tue, 02 Oct 2018 02:37:08 +0000 Gerrit-HasComments: Yes
