You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Lars Volker (Code Review)" <ge...@cloudera.org> on 2017/01/10 14:46:04 UTC
[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template
Lars Volker has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5664
Change subject: IMPALA-4751: Remove blank line from raw_text template
......................................................................
IMPALA-4751: Remove blank line from raw_text template
The additional blank line can break tooling which uses the
/query_profile_encoded endpoint and has been erroneously introduced in
the fix for IMPALA-3918.
Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
---
M tests/webserver/test_web_pages.py
M www/raw_text.tmpl
2 files changed, 12 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/5664/1
--
To view, visit http://gerrit.cloudera.org:8080/5664
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello David Knupp,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5664
to look at the new patch set (#2).
Change subject: IMPALA-4751: Remove blank line from raw_text template
......................................................................
IMPALA-4751: Remove blank line from raw_text template
The additional blank line can break tooling which uses the
/query_profile_encoded endpoint and has been erroneously introduced in
the fix for IMPALA-3918.
Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
---
M tests/webserver/test_web_pages.py
M www/raw_text.tmpl
2 files changed, 16 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/5664/2
--
To view, visit http://gerrit.cloudera.org:8080/5664
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4751: Remove blank line from raw_text template
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5664/1/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:
Line 34: """Test that /query_profile_encoded error message does not have any leading newlines.
Not just no newlines, but the expected line in the case of a missing query.
http://gerrit.cloudera.org:8080/#/c/5664/1/www/raw_text.tmpl
File www/raw_text.tmpl:
Line 18: }}{{contents}}
Any other templates like this that should be fixed?
--
To view, visit http://gerrit.cloudera.org:8080/5664
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4751: Remove blank line from raw_text template
......................................................................
Patch Set 2: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/5664
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-4751: Remove blank line from raw_text template
......................................................................
Patch Set 2:
> DO you want me to start the pre-commit testing Jenkins job?
Thanks for the review. Yes, it would be great if you could run the Jenkins job and submit the change for me.
--
To view, visit http://gerrit.cloudera.org:8080/5664
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-4751: Remove blank line from raw_text template
......................................................................
IMPALA-4751: Remove blank line from raw_text template
The additional blank line can break tooling which uses the
/query_profile_encoded endpoint and has been erroneously introduced in
the fix for IMPALA-3918.
Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Reviewed-on: http://gerrit.cloudera.org:8080/5664
Reviewed-by: Jim Apple <jb...@apache.org>
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M tests/webserver/test_web_pages.py
M www/raw_text.tmpl
2 files changed, 16 insertions(+), 2 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Jim Apple: Looks good to me, approved
Tim Armstrong: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.cloudera.org:8080/5664
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Re: [Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template
Posted by Henry Robinson <he...@apache.org>.
Let's get it in. We can discuss the other issues later.
On Tue, Jan 10, 2017 at 9:33 AM Tim Armstrong (Code Review) <
gerrit@cloudera.org> wrote:
> Tim Armstrong has posted comments on this change.
>
>
>
> Change subject: IMPALA-4751: Remove blank line from raw_text template
>
> ......................................................................
>
>
>
>
>
> Patch Set 2: Code-Review+1
>
>
>
> --
>
> To view, visit http://gerrit.cloudera.org:8080/5664
>
> To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
>
>
>
> Gerrit-MessageType: comment
>
> Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
>
> Gerrit-PatchSet: 2
>
> Gerrit-Project: Impala-ASF
>
> Gerrit-Branch: master
>
> Gerrit-Owner: Lars Volker <lv...@cloudera.com>
>
> Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
>
> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
>
> Gerrit-Reviewer: Impala Public Jenkins
>
> Gerrit-Reviewer: Jim Apple <jb...@apache.org>
>
> Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
>
> Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
>
> Gerrit-HasComments: No
>
>
[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4751: Remove blank line from raw_text template
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/5664
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4751: Remove blank line from raw_text template
......................................................................
Patch Set 2:
Why can't this be fixed on the consumer side? Stripping out leading and trailing white space seems a pretty reasonable requirement for parsing this file.
--
To view, visit http://gerrit.cloudera.org:8080/5664
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4751: Remove blank line from raw_text template
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5664/1/www/raw_text.tmpl
File www/raw_text.tmpl:
Line 17: under the License.
We should also add a comment saying that the whitespace is critical so that nobody makes the same mistake again.
--
To view, visit http://gerrit.cloudera.org:8080/5664
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-4751: Remove blank line from raw_text template
......................................................................
Patch Set 2:
> Why can't this be fixed on the consumer side? Stripping out leading
> and trailing white space seems a pretty reasonable requirement for
> parsing this file.
I suppose it can be done on the consumer side, too. Since this used to work and Impala changed its behavior, it looked like the cleanest approach to me to get the file back into the expected format.
Do we have documentation on these endpoints where we can explain how to parse them and what a consumer should expect? Do you think we should hold of on this change and keep the current behavior instead?
--
To view, visit http://gerrit.cloudera.org:8080/5664
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4751: Remove blank line from raw_text template
......................................................................
Patch Set 2: Code-Review+2
DO you want me to start the pre-commit testing Jenkins job?
--
To view, visit http://gerrit.cloudera.org:8080/5664
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template
Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
David Knupp has posted comments on this change.
Change subject: IMPALA-4751: Remove blank line from raw_text template
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/5664
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-4751: Remove blank line from raw_text template
......................................................................
Patch Set 1:
(3 comments)
Thanks for the reviews, please see PS2.
http://gerrit.cloudera.org:8080/#/c/5664/1/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:
Line 34: """Test that /query_profile_encoded error message does not have any leading newlines.
> Not just no newlines, but the expected line in the case of a missing query.
Done
http://gerrit.cloudera.org:8080/#/c/5664/1/www/raw_text.tmpl
File www/raw_text.tmpl:
Line 17: under the License.
> We should also add a comment saying that the whitespace is critical so that
Done
Line 18: }}{{contents}}
> Any other templates like this that should be fixed?
I manually went through www/*.tmpl and this one was the only one that seemed to have the issue.
--
To view, visit http://gerrit.cloudera.org:8080/5664
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4751: Remove blank line from raw_text template
......................................................................
Patch Set 2:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/158/
--
To view, visit http://gerrit.cloudera.org:8080/5664
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No