Michal Ostrowski 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: (8 comments) http://gerrit.cloudera.org:8080/#/c/11425/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11425/5//COMMIT_MSG@10 PS5, Line 10: profiles. Remove any duplicated newline characters within strings > Please wrap the lines at 72 characters. Done http://gerrit.cloudera.org:8080/#/c/11425/1/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/11425/1/be/src/util/runtime-profile.cc@545 PS1, Line 545: StripTrailingWhitespace(&value); > tab used for whitespace Done http://gerrit.cloudera.org:8080/#/c/11425/1/be/src/util/runtime-profile.cc@549 PS1, Line 549: InfoStrings::iterator it = info_strings_.find(key); > tab used for whitespace Done http://gerrit.cloudera.org:8080/#/c/11425/3/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/11425/3/be/src/util/runtime-profile.cc@547 PS3, Line 547: : lock_guard<SpinLock> l(info_strings_lock_); : InfoStrings::iterator it = info_strings_.find(key); : i > It seems more robust to strip any trailing whitespace by using find_last_no Done 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@545 PS5, Line 545: StripTrailingWhitespace(&value); : StripDupCharacters(&value, '\n', 0); : : lock_guard<SpinLock> l(info_strings_lock_); : I > There is comfortable StripTrailingWhitespace function in Done 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)); > Why is this necessary ? The test code I added gets tripped up because there the runtime profile includes some log messages, and some log messages get formatted with newlines embedded in them, resulting in a double-newline sequence. Removing duplicate newlines allows for the test to assert simply that none are present which is a more worthwhile test than to explicitly and only check for no double-newline after a a "query status: cancelled". 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)); : info_strings_display_order_.push_back(key); : } else { : if (append) { : > strip.h also contains a function for this, see StripDupCharacters. Done http://gerrit.cloudera.org:8080/#/c/11425/5/tests/query_test/test_cancellation.py File tests/query_test/test_cancellation.py: http://gerrit.cloudera.org:8080/#/c/11425/5/tests/query_test/test_cancellation.py@175 PS5, Line 175: close_error = None > nit: unnecessary new line Done -- 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 <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Michal Ostrowski <[email protected]> Gerrit-Comment-Date: Mon, 01 Oct 2018 21:12:53 +0000 Gerrit-HasComments: Yes
