You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michal Ostrowski (Code Review)" <ge...@cloudera.org> on 2018/09/11 22:28:07 UTC
[Impala-ASF-CR] IMPALA-2063 Eliminate redundant newline characters in query status.
Michal Ostrowski has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11425
Change subject: IMPALA-2063 Eliminate redundant newline characters in query status.
......................................................................
IMPALA-2063 Eliminate redundant newline characters in query status.
Prior to this change RuntimeProfile::AddInfoStringInternal had "value"
passed by reference. Subsequently, we needed to conditionally make
a copy of it so as to redact it. Trimming the trailing newline won't work
on a "const std::string &". We need a local copy.
Instead of incurring another copy cost, we can instead recover a hidden
existing copy that currently exists in this function - the storage of
the input parameter into the has table. If instead of using a copy at that
point an std::move operation is used, then we can move the required string
copy to occur at the time "value" is passed into the function.
Hence the "value" parameter can be passed by value, leaving us with a
mutable local copy of the string which we can redact and trim. Subsequently
the local value is moved to permanent storage.
Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
---
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
2 files changed, 19 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/11425/1
--
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: newchange
Gerrit-Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
Gerrit-Change-Number: 11425
Gerrit-PatchSet: 1
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11425 )
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
Patch Set 12: Verified+1
--
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: 12
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Fri, 05 Oct 2018 22:30:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11425 )
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
Patch Set 11:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/966/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 11
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Fri, 05 Oct 2018 19:23:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michal Ostrowski (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11425
to look at the new patch set (#9).
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
IMPALA-2063 Remove newline characters in query status.
Remove extraneous whitespace at the end of strings being added to
profiles. Remove any duplicated newline characters within strings
as well. (The latter step is necessary to allow for a blanket
assertion on this in testing.)
Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
---
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M tests/query_test/test_cancellation.py
3 files changed, 28 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/11425/9
--
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: newpatchset
Gerrit-Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
Gerrit-Change-Number: 11425
Gerrit-PatchSet: 9
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Tue, 02 Oct 2018 02:37:08 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11425 )
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
Patch Set 10:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/963/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 10
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Fri, 05 Oct 2018 17:05:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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:
(1 comment)
The new change looks better now. Looks like you may have missed my comments in PS8. Can you please address them ?
http://gerrit.cloudera.org:8080/#/c/11425/9/tests/query_test/test_cancellation.py
File tests/query_test/test_cancellation.py:
http://gerrit.cloudera.org:8080/#/c/11425/9/tests/query_test/test_cancellation.py@193
PS9, Line 193: the close rpc should have succeeded.
Not sure why this is needed.
--
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 <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Oct 2018 21:13:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michal Ostrowski (Code Review)" <ge...@cloudera.org>.
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 9:
(1 comment)
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.
> Did you mean to push a new patch ? Still the same in PS9.
I haven't pushed a new change yet. I saw your comment here and replied to it after PS9.
--
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: 9
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Oct 2018 22:39:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11425 )
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/840/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 6
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Sep 2018 17:58:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11425 )
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
Patch Set 5:
(4 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: Remove any duplicated newline characters within strings as well. (The latter
Please wrap the lines at 72 characters.
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: // Strip out any whitespace trailing the string.
: size_t end = value.find_last_not_of("\f\r\n\v \t");
: if (end > 0 && end < value.size()) {
: value.resize(end + 1);
: }
There is comfortable StripTrailingWhitespace function in
https://github.com/apache/impala/blob/master/be/src/gutil/strings/strip.h that could do this for you. I am not sure if would consider exactly the same charset as whitespace though.
http://gerrit.cloudera.org:8080/#/c/11425/5/be/src/util/runtime-profile.cc@551
PS5, Line 551: // If there is a newline-like character, remove duplicates
: static std::regex multiple_newline("\n\n+");
: if (std::regex_search(value, multiple_newline)) {
: value = std::regex_replace(value, multiple_newline, "\n");
: }
strip.h also contains a function for this, see StripDupCharacters.
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:
nit: unnecessary new line
--
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: 5
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Sep 2018 16:22:47 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michal Ostrowski (Code Review)" <ge...@cloudera.org>.
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 <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Oct 2018 21:12:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michal Ostrowski (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11425
to look at the new patch set (#7).
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
IMPALA-2063 Remove newline characters in query status.
Remove extraneous whitespace at the end of strings being added to
profiles. Remove any duplicated newline characters within strings
as well. (The latter step is necessary to allow for a blanket
assertion on this in testing.)
Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
---
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M tests/query_test/test_cancellation.py
3 files changed, 25 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/11425/7
--
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: newpatchset
Gerrit-Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
Gerrit-Change-Number: 11425
Gerrit-PatchSet: 7
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 5:
(1 comment)
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: // If there is a newline-like character, remove duplicates
Why is this necessary ?
--
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: 5
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Sep 2018 17:45:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11425 )
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/735/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 5
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Sep 2018 15:10:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michal Ostrowski (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11425
to look at the new patch set (#4).
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
IMPALA-2063 Remove newline characters in query status.
Remove extraneous whitespace at the end of strings being added to profiles.
Remove any duplicated newline characters within strings as well. (The latter
step is necessary to allow for a blanket assertion on this in testing.)
Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
---
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M tests/query_test/test_cancellation.py
3 files changed, 33 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/11425/4
--
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: newpatchset
Gerrit-Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
Gerrit-Change-Number: 11425
Gerrit-PatchSet: 4
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11425 )
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
Patch Set 9:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/916/ : Initial code review checks failed. See linked job for details on the failure.
--
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: 9
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Oct 2018 11:34:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11425 )
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11425/4/tests/query_test/test_cancellation.py
File tests/query_test/test_cancellation.py:
http://gerrit.cloudera.org:8080/#/c/11425/4/tests/query_test/test_cancellation.py@189
PS4, Line 189: n
flake8: E713 test for membership should be 'not in'
http://gerrit.cloudera.org:8080/#/c/11425/4/tests/query_test/test_cancellation.py@189
PS4, Line 189: %
flake8: E501 line too long (98 > 90 characters)
--
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: 4
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Sep 2018 10:38:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11425 )
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
Patch Set 1:
(2 comments)
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: Redact(&value);
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/11425/1/be/src/util/runtime-profile.cc@549
PS1, Line 549: value.resize(size - 1);
tab used for whitespace
--
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: 1
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Sep 2018 23:55:16 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 9:
(2 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.
> Done
Did you mean to push a new patch ? Still the same in PS9.
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"
> Done
Did you mean to push a new patch ? Still the same in PS9.
--
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: 9
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Oct 2018 21:32:46 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michal Ostrowski (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11425
to look at the new patch set (#6).
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
IMPALA-2063 Remove newline characters in query status.
Remove extraneous whitespace at the end of strings being added to profiles.
Remove any duplicated newline characters within strings as well. (The latter
step is necessary to allow for a blanket assertion on this in testing.)
Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
---
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M tests/query_test/test_cancellation.py
3 files changed, 25 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/11425/6
--
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: newpatchset
Gerrit-Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
Gerrit-Change-Number: 11425
Gerrit-PatchSet: 6
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11425 )
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/725/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 4
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Sep 2018 00:12:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 5:
(1 comment)
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: // If there is a newline-like character, remove duplicates
> The test for double newlines effectively was a test that info strings aren'
Actually, it's possible to get the value of individual field with the Thrift profile. So, it's definitely possible to just extract the "Query Status" field from the final profile.
Copying some code from test_observability.py:
tree = self.impalad_test_service.get_thrift_profile(query_id)
if tree:
query_status = tree.nodes[1].info_strings["Query Status"]
assert query_status == query_status.rstrip()
--
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: 5
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Tue, 02 Oct 2018 19:30:08 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michal Ostrowski (Code Review)" <ge...@cloudera.org>.
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:
(3 comments)
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
> Done
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));
> I applied the patch and looked at the query profile output at the debug web
That's fine. The only reason the double newline is being stripped is because it's the only way to make a sane test.
If I take this out, I have to remove the test code as well -- and I really don't see the value of adding in a specific test to validate that "Status: CANCELLED" doesn't have extra newlines (especially since we don't test for any other formatting issues).
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) {
:
> Done
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 <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Tue, 02 Oct 2018 12:25:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11425 )
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/644/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 1
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Sep 2018 23:02:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/883/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Oct 2018 20:37:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michal Ostrowski (Code Review)" <ge...@cloudera.org>.
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 9:
(4 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 stripp
Done
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
Done
http://gerrit.cloudera.org:8080/#/c/11425/8/be/src/util/runtime-profile.cc@541
PS8, Line 541: if (redact) Redact(&value);
:
: S
> nit: can fit in one line
Done
http://gerrit.cloudera.org:8080/#/c/11425/8/be/src/util/runtime-profile.cc@546
PS8, Line 546: InfoStrings::iterator it = info_stri
> Please see comments elsewhere. This change in behavior seems to be more tha
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: 9
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Oct 2018 21:22:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11425 )
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/647/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 3
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 Sep 2018 00:49:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michal Ostrowski (Code Review)" <ge...@cloudera.org>.
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 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11425/10/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:
http://gerrit.cloudera.org:8080/#/c/11425/10/be/src/util/runtime-profile.cc@41
PS10, Line 41:
> nit: blank 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: 11
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Fri, 05 Oct 2018 18:33:31 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 11: Code-Review+2
--
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: 11
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Fri, 05 Oct 2018 18:33:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 12: Code-Review+2
--
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: 12
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Fri, 05 Oct 2018 18:34:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11425 )
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
IMPALA-2063 Remove newline characters in query status.
Remove extraneous whitespace at the end of strings being added to
profiles. Remove any duplicated newline characters within strings
as well. (The latter step is necessary to allow for a blanket
assertion on this in testing.)
Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
Reviewed-on: http://gerrit.cloudera.org:8080/11425
Reviewed-by: Michael Ho <kw...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M tests/query_test/test_cancellation.py
3 files changed, 29 insertions(+), 8 deletions(-)
Approvals:
Michael Ho: Looks good to me, approved
Impala Public Jenkins: Verified
--
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: merged
Gerrit-Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
Gerrit-Change-Number: 11425
Gerrit-PatchSet: 13
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michal Ostrowski (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11425
to look at the new patch set (#10).
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
IMPALA-2063 Remove newline characters in query status.
Remove extraneous whitespace at the end of strings being added to
profiles. Remove any duplicated newline characters within strings
as well. (The latter step is necessary to allow for a blanket
assertion on this in testing.)
Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
---
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M tests/query_test/test_cancellation.py
3 files changed, 30 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/11425/10
--
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: newpatchset
Gerrit-Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
Gerrit-Change-Number: 11425
Gerrit-PatchSet: 10
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michal Ostrowski (Code Review)" <ge...@cloudera.org>.
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 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11425/9/tests/query_test/test_cancellation.py
File tests/query_test/test_cancellation.py:
http://gerrit.cloudera.org:8080/#/c/11425/9/tests/query_test/test_cancellation.py@193
PS9, Line 193: # "Plan" text may be strangely formatted.
> Not sure why this is needed.
To explain why "Plan" is excluded from the check below.
--
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: 9
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Oct 2018 21:27:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michal Ostrowski (Code Review)" <ge...@cloudera.org>.
Michal Ostrowski has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/11425 )
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
IMPALA-2063 Remove newline characters in query status.
Prior to this change RuntimeProfile::AddInfoStringInternal had "value"
passed by reference. Subsequently, we needed to conditionally make
a copy of it so as to redact it. Trimming the trailing newline won't work
on a "const std::string &". We need a local copy.
Instead of incurring another copy cost, we can instead recover a hidden
existing copy that currently exists in this function - the storage of
the input parameter into the has table. If instead of using a copy at that
point an std::move operation is used, then we can move the required string
copy to occur at the time "value" is passed into the function.
Hence the "value" parameter can be passed by value, leaving us with a
mutable local copy of the string which we can redact and trim. Subsequently
the local value is moved to permanent storage.
Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
---
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
2 files changed, 19 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/11425/2
--
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: newpatchset
Gerrit-Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
Gerrit-Change-Number: 11425
Gerrit-PatchSet: 2
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michal Ostrowski (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11425
to look at the new patch set (#3).
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
IMPALA-2063 Remove newline characters in query status.
Prior to this change RuntimeProfile::AddInfoStringInternal had "value"
passed by reference. Subsequently, we needed to conditionally make
a copy of it so as to redact it. Trimming the trailing newline won't work
on a "const std::string &". We need a local copy.
Instead of incurring another copy cost, we can instead recover a hidden
existing copy that currently exists in this function - the storage of
the input parameter into the has table. If instead of using a copy at that
point an std::move operation is used, then we can move the required string
copy to occur at the time "value" is passed into the function.
Hence the "value" parameter can be passed by value, leaving us with a
mutable local copy of the string which we can redact and trim. Subsequently
the local value is moved to permanent storage.
Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
---
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
2 files changed, 19 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/11425/3
--
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: newpatchset
Gerrit-Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
Gerrit-Change-Number: 11425
Gerrit-PatchSet: 3
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11425 )
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
Patch Set 12:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3285/ DRY_RUN=false
--
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: 12
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Fri, 05 Oct 2018 18:34:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michal Ostrowski (Code Review)" <ge...@cloudera.org>.
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:
(1 comment)
> Patch Set 5:
>
> (1 comment)
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));
> I guess a more meaningful test is to verify that all "Query Status" (not ju
The test for double newlines effectively was a test that info strings aren't newline terminated, since the construction of the full profile text inserts newlines between each info string. As I pointed out earlier, that doesn't work because there are other strings that will break that test without this change. More importantly, we only get to test the final profile text and not the individual strings that it is built from, so we don't have the context to identify if a newline is from an info string or if it was introduced in formatting.
There is no existing tests to validate correct formatting of this text.
The text overall is a free-for-all -- we don't exactly know how and where all possible strings come from, making it hard to
impose and then verify formatting rules. (How do we know that our tests simply never generate the particular string that would break the profile text verification?)
Adding more specific tests in turn would result in us effectively writing tests to match our implementation (i.e. string matches).
FWIW, here's an example of a problematic profile text -- note the "WARNING" towards the end.
Query (id=324df3273ce7648d:8d1602ea00000000):
DEBUG MODE WARNING: Query profile created while running a DEBUG build of Impala. Use RELEASE builds to measure query performance.
Summary:
Session ID: ea417f33ee6aee28:58213ba9cfaaf191
Session Type: BEESWAX
Start Time: 2018-10-02 11:16:51.674327000
End Time: 2018-10-02 11:16:52.681041000
Query Type: QUERY
Query State: EXCEPTION
Query Status: Cancelled
Impala Version: impalad version 3.1.0-SNAPSHOT DEBUG (build 6247d6d41cfea7149704cf9345d014c17375f61b)
User: mostrows
Connected User: mostrows
Delegated User:
Network Address: ::ffff:127.0.0.1:51148
Default Db: tpch_seq_snap
Sql Statement: select count(l_returnflag) pk from lineitem
Coordinator: mostrows-dev.vpc.cloudera.com:22000
Query Options (set by configuration): ABORT_ON_ERROR=1,BUFFER_POOL_LIMIT=0,EXEC_SINGLE_NODE_ROWS_THRESHOLD=0,DISABLE_CODEGEN_ROWS_THRESHOLD=0,CPU_LIMIT_S=100000
Query Options (set by configuration and planner): ABORT_ON_ERROR=1,BUFFER_POOL_LIMIT=0,EXEC_SINGLE_NODE_ROWS_THRESHOLD=0,MT_DOP=0,DISABLE_CODEGEN_ROWS_THRESHOLD=0,CPU_LIMIT_S=100000
Plan:
----------------
Max Per-Host Resource Reservation: Memory=8.00MB Threads=3
Per-Host Resource Estimates: Memory=236MB
WARNING: The following tables are missing relevant table and/or column statistics.
tpch_seq_snap.lineitem
F01:PLAN FRAGMENT [UNPARTITIONED] hosts=1 instances=1
| Per-Host Resources: mem-estimate=10.00MB mem-reservation=0B thread-reservation=1
PLAN-ROOT SINK
--
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 <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Tue, 02 Oct 2018 18:19:22 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michal Ostrowski (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11425
to look at the new patch set (#11).
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
IMPALA-2063 Remove newline characters in query status.
Remove extraneous whitespace at the end of strings being added to
profiles. Remove any duplicated newline characters within strings
as well. (The latter step is necessary to allow for a blanket
assertion on this in testing.)
Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
---
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M tests/query_test/test_cancellation.py
3 files changed, 29 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/11425/11
--
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: newpatchset
Gerrit-Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
Gerrit-Change-Number: 11425
Gerrit-PatchSet: 11
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 10: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11425/10/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:
http://gerrit.cloudera.org:8080/#/c/11425/10/be/src/util/runtime-profile.cc@41
PS10, Line 41:
nit: blank line
--
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: 10
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Fri, 05 Oct 2018 18:24:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michal Ostrowski (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11425
to look at the new patch set (#5).
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
IMPALA-2063 Remove newline characters in query status.
Remove extraneous whitespace at the end of strings being added to profiles.
Remove any duplicated newline characters within strings as well. (The latter
step is necessary to allow for a blanket assertion on this in testing.)
Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
---
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M tests/query_test/test_cancellation.py
3 files changed, 34 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/11425/5
--
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: newpatchset
Gerrit-Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
Gerrit-Change-Number: 11425
Gerrit-PatchSet: 5
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michal Ostrowski (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11425
to look at the new patch set (#8).
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
IMPALA-2063 Remove newline characters in query status.
Remove extraneous whitespace at the end of strings being added to
profiles. Remove any duplicated newline characters within strings
as well. (The latter step is necessary to allow for a blanket
assertion on this in testing.)
Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
---
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M tests/query_test/test_cancellation.py
3 files changed, 24 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/11425/8
--
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: newpatchset
Gerrit-Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
Gerrit-Change-Number: 11425
Gerrit-PatchSet: 8
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 3:
(1 comment)
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: size_t size = value.size();
: if (size > 0 && value.at(size - 1) == '\n') {
: value.resize(size - 1);
: }
It seems more robust to strip any trailing whitespace by using find_last_not_of(" \t\f\v\n\r") to find the last non-whitespace char and use substr() to extract the part which is valid. Just a suggestion, not dictating the implementation.
--
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: 3
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Sep 2018 20:47:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 5:
(1 comment)
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: // If there is a newline-like character, remove duplicates
> That's fine. The only reason the double newline is being stripped is becau
I guess a more meaningful test is to verify that all "Query Status" (not just Status: Cancelled) (or in general any info string) don't end with whitespace / newline. I can see either incorporating that verification into some test infrastructure (e.g. somewhere in test_result_verifier.py) or you write a test which triggers different types of query statuses (e.g. OK, mem limit exceeded, scan error, cancelled etc) to verify that they don't end with whitespaces. The former seems to be more 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: 5
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Tue, 02 Oct 2018 17:56:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11425 )
Change subject: IMPALA-2063 Remove newline characters in query status.
......................................................................
Patch Set 9:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/922/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 9
Gerrit-Owner: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Oct 2018 23:38:35 +0000
Gerrit-HasComments: No