[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14103 )

Change subject: IMPALA-8858: Add metrics tracking num queries running on 
executor groups
..


Patch Set 16: Code-Review+2

(2 comments)

LGTM, a few typos that could be ignored

http://gerrit.cloudera.org:8080/#/c/14103/16//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14103/16//COMMIT_MSG@10
PS16, Line 10: will have a metric added that display the number of queries
Nit: displays


http://gerrit.cloudera.org:8080/#/c/14103/16/tests/custom_cluster/test_executor_groups.py
File tests/custom_cluster/test_executor_groups.py:

http://gerrit.cloudera.org:8080/#/c/14103/16/tests/custom_cluster/test_executor_groups.py@354
PS16, Line 354: # Create and exec group of min size 2 to exercise the case 
where a group becomes
Nit: Create an exec group...



--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 16
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Wed, 18 Sep 2019 18:35:12 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8806: Add metrics to improve observability of executor groups

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13979 )

Change subject: IMPALA-8806: Add metrics to improve observability of executor 
groups
..


Patch Set 4: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/13979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7745ea1c7c6778d3fb5e59adbc873697beb0f3b9
Gerrit-Change-Number: 13979
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 05 Aug 2019 21:37:30 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8806: Add metrics to improve observability of executor groups

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13979 )

Change subject: IMPALA-8806: Add metrics to improve observability of executor 
groups
..


Patch Set 2: Code-Review+1

(3 comments)

Only giving +1 as it does not compile but LGTM and we need this very soon :-)

http://gerrit.cloudera.org:8080/#/c/13979/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/13979/2//COMMIT_MSG@11
PS2, Line 11: that have atleast one live executor, number of executor groups 
that are
typo -> at least


http://gerrit.cloudera.org:8080/#/c/13979/2/common/thrift/metrics.json
File common/thrift/metrics.json:

http://gerrit.cloudera.org:8080/#/c/13979/2/common/thrift/metrics.json@2456
PS2, Line 2456: "description": "Total number of executor groups that are in 
a healthy state",
I found the difference between "at least one executor" and "healthy" a bit 
confusing, could we add text somewhere to explain healthy means having at least 
the configured minimum number of executors. It also may be worth noting 
somewhere that cluster-membership.executor-groups.total >= 
cluster-membership.executor-groups.total-healthy


http://gerrit.cloudera.org:8080/#/c/13979/2/tests/custom_cluster/test_auto_scaling.py
File tests/custom_cluster/test_auto_scaling.py:

http://gerrit.cloudera.org:8080/#/c/13979/2/tests/custom_cluster/test_auto_scaling.py@70
PS2, Line 70:   assert any(self._get_num_backends() >= GROUP_SIZE + 1 or 
sleep(1)
The change here is because previously we were fetching number of executors, now 
we are fetching number of backends? So the +1 is because we have 1 coordinator? 
Might be worth a comment somewhere. Is there a metric for number of 
coordinators?  [but don't delay this change adding this :-) ]



--
To view, visit http://gerrit.cloudera.org:8080/13979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7745ea1c7c6778d3fb5e59adbc873697beb0f3b9
Gerrit-Change-Number: 13979
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sun, 04 Aug 2019 19:39:14 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8615: [DOCS] Document the scalable admission control parameters

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13906 )

Change subject: IMPALA-8615: [DOCS] Document the scalable admission control 
parameters
..


Patch Set 2:

(1 comment)

This basically looks good, one thing caught my attention

http://gerrit.cloudera.org:8080/#/c/13906/2/docs/topics/impala_admission.xml
File docs/topics/impala_admission.xml:

http://gerrit.cloudera.org:8080/#/c/13906/2/docs/topics/impala_admission.xml@114
PS2, Line 114: This setting
 : scales with the number of hosts in the resource pool.
Instead of 'hosts' I think you should say 'executors', to be consistent.
Does the setting scale? I would say something like 'The effect of this setting 
scales with...'



--
To view, visit http://gerrit.cloudera.org:8080/13906
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibca9cf9586359ee0f1ce0dd8744b4709752a26f1
Gerrit-Change-Number: 13906
Gerrit-PatchSet: 2
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Wed, 24 Jul 2019 16:54:58 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8615: [DOCS] Document the scalable admission control parameters

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13906 )

Change subject: IMPALA-8615: [DOCS] Document the scalable admission control 
parameters
..


Patch Set 3: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/13906
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibca9cf9586359ee0f1ce0dd8744b4709752a26f1
Gerrit-Change-Number: 13906
Gerrit-PatchSet: 3
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Wed, 24 Jul 2019 20:45:58 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5149: Provide query profile in JSON format

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
..


Patch Set 8:

Should the json include the explicit version number of the format?
 "version: "1.0"
to help people writing parsers?
Like Bharath (and DavidR in the design doc) I would like to see more about 
future compatibility guarantees


--
To view, visit http://gerrit.cloudera.org:8080/13801
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
Gerrit-Change-Number: 13801
Gerrit-PatchSet: 8
Gerrit-Owner: Jiawei Wang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: David Rorke 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jiawei Wang 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Sahil Takiar 
Gerrit-Comment-Date: Mon, 22 Jul 2019 17:50:15 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14103 )

Change subject: IMPALA-8858: Add metrics tracking num queries running on 
executor groups
..


Patch Set 19: Code-Review+2

Reviewed ordering-in-test results changes


--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 19
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Thu, 19 Sep 2019 22:30:26 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8928: Add MEM LIMIT EXECUTORS query option

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14294 )

Change subject: IMPALA-8928: Add MEM_LIMIT_EXECUTORS query option
..


Patch Set 1: Code-Review+2

(2 comments)

This looks nice and simple.

http://gerrit.cloudera.org:8080/#/c/14294/1/be/src/scheduling/query-schedule.cc
File be/src/scheduling/query-schedule.cc:

http://gerrit.cloudera.org:8080/#/c/14294/1/be/src/scheduling/query-schedule.cc@344
PS1, Line 344:   if (!has_query_option && 
query_options().__isset.mem_limit_executors
I had to look at a broader context to understand that 'has_query_limit'  means 
'has the MEM_LIMIT query option', not just any query option.


http://gerrit.cloudera.org:8080/#/c/14294/1/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:

http://gerrit.cloudera.org:8080/#/c/14294/1/common/thrift/ImpalaService.thrift@482
PS1, Line 482:   // otherwise specified either as:
Nit: capitalize 'Otherwise'



--
To view, visit http://gerrit.cloudera.org:8080/14294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I20dfd4e8fc7ffd9130db9f942efa78965c724e18
Gerrit-Change-Number: 14294
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 25 Sep 2019 23:11:56 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8065 Edit OS version and Kernel version in OSInfo

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14531 )

Change subject: IMPALA-8065 Edit OS version and Kernel version in OSInfo
..


Patch Set 6:

(2 comments)

Still a few things to think about...

http://gerrit.cloudera.org:8080/#/c/14531/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14531/6//COMMIT_MSG@7
PS6, Line 7: IMPALA-8065 Edit OS version and Kernel version in OSInfo
Can you improve this one line description? Why is it Edit?


http://gerrit.cloudera.org:8080/#/c/14531/6/be/src/util/os-info.cc
File be/src/util/os-info.cc:

http://gerrit.cloudera.org:8080/#/c/14531/6/be/src/util/os-info.cc@72
PS6, Line 72:   if (fields[0].compare("PRETTY_NAME") == 0) {
Will this work on Centos6?
https://www.liquidweb.com/kb/how-to-check-your-centos-version/



--
To view, visit http://gerrit.cloudera.org:8080/14531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I848c9e53ee4e0bf8ae0874bb6da28e8efa7f7c8a
Gerrit-Change-Number: 14531
Gerrit-PatchSet: 6
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Fri, 01 Nov 2019 18:08:07 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8065 Change the format OS version and Kernel version dispalyed in OSInfo

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14531 )

Change subject: IMPALA-8065 Change the format OS version and Kernel version 
dispalyed in OSInfo
..


Patch Set 7:

(3 comments)

Getting really close :-)

http://gerrit.cloudera.org:8080/#/c/14531/7//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14531/7//COMMIT_MSG@26
PS7, Line 26: debian. Each OS picked one version to test.
I think you must have some some extra testing on Centos6, you could add a not 
about it here


http://gerrit.cloudera.org:8080/#/c/14531/7/be/src/util/os-info-test.cc
File be/src/util/os-info-test.cc:

http://gerrit.cloudera.org:8080/#/c/14531/7/be/src/util/os-info-test.cc@27
PS7, Line 27:   ASSERT_NE(osinfo.kernel_version(), "Unknown");
Add a similar test for fast_clock()?


http://gerrit.cloudera.org:8080/#/c/14531/7/be/src/util/os-info.cc
File be/src/util/os-info.cc:

http://gerrit.cloudera.org:8080/#/c/14531/7/be/src/util/os-info.cc@67
PS7, Line 67: os_version_.erase(remove(os_version_.begin(), 
os_version_.end(), '\"'),
Add a 1-line comment explaining what is happening?
You may want to run clang-format again on this method.



--
To view, visit http://gerrit.cloudera.org:8080/14531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I848c9e53ee4e0bf8ae0874bb6da28e8efa7f7c8a
Gerrit-Change-Number: 14531
Gerrit-PatchSet: 7
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Tue, 05 Nov 2019 19:52:15 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8065 Edit OS version and Kernel version in OSInfo

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14531 )

Change subject: IMPALA-8065 Edit OS version and Kernel version in OSInfo
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14531/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14531/3//COMMIT_MSG@15
PS3, Line 15: OS version: "Ubuntu 16.04.6 LTS"
> That's coming from the original file.
OK, but we should make it pretty



--
To view, visit http://gerrit.cloudera.org:8080/14531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I848c9e53ee4e0bf8ae0874bb6da28e8efa7f7c8a
Gerrit-Change-Number: 14531
Gerrit-PatchSet: 3
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Thu, 31 Oct 2019 18:58:02 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8065 Add OS version and Kernel version in OSInfo

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14531 )

Change subject: IMPALA-8065 Add OS version and Kernel version in OSInfo
..


Patch Set 3:

(8 comments)

Code looks good, Comments and commit message need more work, I think

http://gerrit.cloudera.org:8080/#/c/14531/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14531/3//COMMIT_MSG@8
PS3, Line 8:
I think it is important to write a commit message that can be understood
by someone who has not read the code you are changing. Sometimes it
helps to explain what the code is.

OsInfo is used to get human readable information about the OS on which
Impala is running.

Before this change OsInfo::DebugString() would print two lines:
- OS version: the long name of the Linux kernel from /proc/version
- Clock: the type of clock used
After this change OsInfo::DebugString() will print three lines:
- OS version: the short name of the Linux distribution.
  If Docker is being used this is the name of the Container OS
- Kernel version: the long name of the Linux kernel from /proc/version
  If Docker is being used this is the description of the Host OS.
- Clock: the type of clock used


http://gerrit.cloudera.org:8080/#/c/14531/3//COMMIT_MSG@15
PS3, Line 15: OS version: "Ubuntu 16.04.6 LTS"
Why does OS Version have quotes but Kernel version does not?


http://gerrit.cloudera.org:8080/#/c/14531/3/be/src/util/CMakeLists.txt
File be/src/util/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/14531/3/be/src/util/CMakeLists.txt@124
PS3, Line 124:   os-info-test.cc
These should be in alphabetical order


http://gerrit.cloudera.org:8080/#/c/14531/3/be/src/util/CMakeLists.txt@187
PS3, Line 187: ADD_UNIFIED_BE_LSAN_TEST(os-info-test "OsInfo.*")
These should be in alphabetical order


http://gerrit.cloudera.org:8080/#/c/14531/3/be/src/util/os-info-test.cc
File be/src/util/os-info-test.cc:

http://gerrit.cloudera.org:8080/#/c/14531/3/be/src/util/os-info-test.cc@25
PS3, Line 25:   ASSERT_NE(osinfo.os_version(), "Unknown");
Add a simple comment explaining the test


http://gerrit.cloudera.org:8080/#/c/14531/3/be/src/util/os-info.h
File be/src/util/os-info.h:

http://gerrit.cloudera.org:8080/#/c/14531/3/be/src/util/os-info.h@35
PS3, Line 35:   /// Name of the OS release.
Simple name of the OS.
If Docker is used this is the name of the Container OS


http://gerrit.cloudera.org:8080/#/c/14531/3/be/src/util/os-info.h@42
PS3, Line 42:   /// and the version of a GCC compiler used to build it.
The version of Linux kernel and the version of the compiler used to build it.
If Docker is used this is the Host OS.


http://gerrit.cloudera.org:8080/#/c/14531/3/be/src/util/os-info.cc
File be/src/util/os-info.cc:

http://gerrit.cloudera.org:8080/#/c/14531/3/be/src/util/os-info.cc@62
PS3, Line 62: os_path = "/etc/centos-release";
Add a comment saying "Only old distributions like Centos 6"



--
To view, visit http://gerrit.cloudera.org:8080/14531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I848c9e53ee4e0bf8ae0874bb6da28e8efa7f7c8a
Gerrit-Change-Number: 14531
Gerrit-PatchSet: 3
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Thu, 31 Oct 2019 00:06:28 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8065 Change the format OS version and Kernel version dispalyed in OSInfo

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14531 )

Change subject: IMPALA-8065 Change the format OS version and Kernel version 
dispalyed in OSInfo
..


Patch Set 8:

The code all seems OK. I am still not happy with the names of the fields.
I think it is an unnecessary risk to change the meaning of "OS version:" and it 
should continue to be the long name of the Linux kernel from /proc/version.
For the short name then we need a new name, what about "OS Release" or "OS 
Distribution" ?


--
To view, visit http://gerrit.cloudera.org:8080/14531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I848c9e53ee4e0bf8ae0874bb6da28e8efa7f7c8a
Gerrit-Change-Number: 14531
Gerrit-PatchSet: 8
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Sat, 16 Nov 2019 23:33:11 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8065: Add OS distribution name in OSInfo

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14531 )

Change subject: IMPALA-8065: Add OS distribution name in OSInfo
..


Patch Set 9: Code-Review+2

Thanks Xiaomeng LGTM


--
To view, visit http://gerrit.cloudera.org:8080/14531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I848c9e53ee4e0bf8ae0874bb6da28e8efa7f7c8a
Gerrit-Change-Number: 14531
Gerrit-PatchSet: 9
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Fri, 22 Nov 2019 19:33:35 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9151: Maintain cluster size in ExecutorMembershipSnapshot

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14756 )

Change subject: IMPALA-9151: Maintain cluster size in ExecutorMembershipSnapshot
..


Patch Set 1: Code-Review+2

(5 comments)

LGTM I have a few nits

http://gerrit.cloudera.org:8080/#/c/14756/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14756/1//COMMIT_MSG@18
PS1, Line 18: join vs a PHJ.
Maybe spell out "partitioned hash join" (if that is what this is)?
Update: I see this in a few places, as a newbie I like seeing things spelled 
out.


http://gerrit.cloudera.org:8080/#/c/14756/1//COMMIT_MSG@21
PS1, Line 21: planner will use a configurable default which should be set to the
Maybe mention that the default is 20?


http://gerrit.cloudera.org:8080/#/c/14756/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/14756/1/be/src/service/impala-server.cc@253
PS1, Line 253: DEFINE_int32(num_expected_executors, 20, "The number of 
executors that the planner "
Maybe 'the number of executors that are expected to be available for the 
execution of a single query'


http://gerrit.cloudera.org:8080/#/c/14756/1/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java
File fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java:

http://gerrit.cloudera.org:8080/#/c/14756/1/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java@39
PS1, Line 39:   // The set of hosts that are members of the cluster given by 
hostname.
Should we add some caveats here (and for ipAddresses_) that these values are 
only present if we are not using executor groups (IIUC)


http://gerrit.cloudera.org:8080/#/c/14756/1/fe/src/test/java/org/apache/impala/planner/ClusterSizeTest.java
File fe/src/test/java/org/apache/impala/planner/ClusterSizeTest.java:

http://gerrit.cloudera.org:8080/#/c/14756/1/fe/src/test/java/org/apache/impala/planner/ClusterSizeTest.java@83
PS1, Line 83: // Adding two or more executors will make the planner switch 
to a PHJ.
partitioned hash join



--
To view, visit http://gerrit.cloudera.org:8080/14756
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6b05326c82fb3ca625c015cfcdc38f891f5d4f9
Gerrit-Change-Number: 14756
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Fri, 22 Nov 2019 00:34:19 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7550: Add documentation to profile counters

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14837 )

Change subject: IMPALA-7550: Add documentation to profile counters
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14837/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14837/1//COMMIT_MSG@9
PS1, Line 9: This is the work based on Lars' experiment on
Can you update the commit message to be standalone?
I would instead like to see a high level overview of how to use the 
counters/documentation



--
To view, visit http://gerrit.cloudera.org:8080/14837
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I72d2380eff95b8d982ce23b9f0252810b495f355
Gerrit-Change-Number: 14837
Gerrit-PatchSet: 1
Gerrit-Owner: Jiawei Wang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jiawei Wang 
Gerrit-Comment-Date: Wed, 04 Dec 2019 22:39:49 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Update gitignore files

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14858 )

Change subject: Update gitignore files
..


Patch Set 1: Code-Review+2

LGTM


--
To view, visit http://gerrit.cloudera.org:8080/14858
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I357441fab00ac031fbc70c40e4574e7a723fdedd
Gerrit-Change-Number: 14858
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Fri, 06 Dec 2019 00:09:22 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8571: harden QueryEventHook execution

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13748 )

Change subject: IMPALA-8571: harden QueryEventHook execution
..


Patch Set 28:

(5 comments)

Just a few replies to your useful comments.
Waiting for next patch before doing more thinking

http://gerrit.cloudera.org:8080/#/c/13748/28//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/13748/28//COMMIT_MSG@19
PS28, Line 19: See Java Thread.setDaemon(boolean) for what a "daemon thread" 
entails.
> I think it depends on whether or not they feel that query hook execution sh
If I'm a user I don't want to read javadoc, I need help deciding. The flag is 
not even an advanced flag so sooner or later someone will have to explain the 
tradeoffs. This is the first actual change mentioned in the commit message so 
it must be important?


http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java
File 
fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java:

http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@82
PS28, Line 82:  * execute. You then create an executor with a thread pool of 
size 1 and a hook
> Flashbacks to my technical-writing course :)
:-)


http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@182
PS28, Line 182: final ArrayBlockingQueue boundedQueue =
> You mean declare the var as the most general type possible, right?  Done.
Yes, thanks


http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@290
PS28, Line 290: t);
> Yep, in fact it will log the stack trace this way! Does look weird and I al
Thanks


http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:

http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/service/Frontend.java@1624
PS28, Line 1624:* for {@link QueryEventHook}, which is part of the 
published api.
> So the `QueryEventHook` class is published in a separate jar than the rest
Thanks for thinking about this. I can see why the jar (and hence the api) is a 
separate artifact.
My question is where the comments will be most useful.
To me it seems like developers are most likely to be reading impala code than 
to be reading the api code.



--
To view, visit http://gerrit.cloudera.org:8080/13748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb88422f7cfe86947d11ce57d2b4c63e57d1b643
Gerrit-Change-Number: 13748
Gerrit-PatchSet: 28
Gerrit-Owner: radford nguyen 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: radford nguyen 
Gerrit-Comment-Date: Mon, 21 Oct 2019 16:34:40 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7504 ParseKerberosPrincipal() should use krb5 parse name() instead

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14433 )

Change subject: IMPALA-7504 ParseKerberosPrincipal() should use 
krb5_parse_name() instead
..


Patch Set 4:

(1 comment)

Commit message still needs improvement

http://gerrit.cloudera.org:8080/#/c/14433/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14433/4//COMMIT_MSG@7
PS4, Line 7: IMPALA-7504 ParseKerberosPrincipal() should use krb5_parse_name() 
instead
See comments on commit msg in patch set 2.



--
To view, visit http://gerrit.cloudera.org:8080/14433
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e64ebdc10f102dbdc5b87f6fe3f2a0310b1be24
Gerrit-Change-Number: 14433
Gerrit-PatchSet: 4
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Mon, 21 Oct 2019 20:24:20 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7504/KUDU-2979 ParseKerberosPrincipal() should use krb5 parse name() instead

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14433 )

Change subject: IMPALA-7504/KUDU-2979 ParseKerberosPrincipal() should use 
krb5_parse_name() instead
..


Patch Set 5:

(7 comments)

This is all looking close. I have some picky comments

http://gerrit.cloudera.org:8080/#/c/14433/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14433/5//COMMIT_MSG@10
PS5, Line 10: creating our own.
instead of using custom code.


http://gerrit.cloudera.org:8080/#/c/14433/5//COMMIT_MSG@12
PS5, Line 12: As src/kudu/security/init.cc already have g_krb5_ctx initialized,
The reader of commit messages is developers like you and me.
Will the reader know what 'g_krb5_ctx' is?
I think you want to say something like:
"When kerberos is initialized in Impala's copy of Kudu code, it stores a global 
context which is used when accessing the Krb5 library. To use this global 
context the code that parses the principal name is moved into the Impala Kudu 
code. This new code is then called from the existing ParseKerberosPrincipal 
method."


http://gerrit.cloudera.org:8080/#/c/14433/5//COMMIT_MSG@18
PS5, Line 18: Add an authentication-test to verify principal with special 
character.
to verify parsing a principal nae containing a special character.


http://gerrit.cloudera.org:8080/#/c/14433/5//COMMIT_MSG@19
PS5, Line 19: Manually tested with bad format principal, throw out error code 2
Is it possible to have an automated test for this?


http://gerrit.cloudera.org:8080/#/c/14433/5//COMMIT_MSG@21
PS5, Line 21:
Do you want to mention running end-to-end tests?


http://gerrit.cloudera.org:8080/#/c/14433/5/be/src/kudu/security/init.h
File be/src/kudu/security/init.h:

http://gerrit.cloudera.org:8080/#/c/14433/5/be/src/kudu/security/init.h@39
PS5, Line 39: // Convert a string representation of a principal name to a 
krb5_principal structure.
Maybe: "Parse a kerberos principal name and extract the ervice_name, hostname, 
and realm from it."


http://gerrit.cloudera.org:8080/#/c/14433/5/be/src/util/auth-util.cc
File be/src/util/auth-util.cc:

http://gerrit.cloudera.org:8080/#/c/14433/5/be/src/util/auth-util.cc@92
PS5, Line 92:   hostname, realm),"bad principal format " + principal);
It is more idiomatic and marginally more efficient to use:
Substitute("bad principal name $0", principal)



--
To view, visit http://gerrit.cloudera.org:8080/14433
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e64ebdc10f102dbdc5b87f6fe3f2a0310b1be24
Gerrit-Change-Number: 14433
Gerrit-PatchSet: 5
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Tue, 22 Oct 2019 00:08:11 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9034: fix distcc+ccache

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14408 )

Change subject: IMPALA-9034: fix distcc+ccache
..


Patch Set 3: Code-Review+2

(1 comment)

Thanks for fixing the lock-up

http://gerrit.cloudera.org:8080/#/c/14408/3/bin/distcc/distcc.sh
File bin/distcc/distcc.sh:

http://gerrit.cloudera.org:8080/#/c/14408/3/bin/distcc/distcc.sh@41
PS3, Line 41:   echo "Expected IMPALA_DISTCC_ENV_VERSION=$EXPECTED_VERSION, 
re-source distcc_env.sh"
Nice



--
To view, visit http://gerrit.cloudera.org:8080/14408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0b080709bd765056b9296d3deb805038fc01e5d
Gerrit-Change-Number: 14408
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 15 Oct 2019 22:30:57 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7504 ParseKerberosPrincipal() should use krb5 parse name() instead

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14433 )

Change subject: IMPALA-7504 ParseKerberosPrincipal() should use 
krb5_parse_name() instead
..


Patch Set 2:

(10 comments)

Thanks for a first cut at this.
I'm still not 100% convinced that this is the right thing to do. but whatever 
happens it is a good training exercise.
I think the general questions are:
What is the performance impact of this change. Is this code on the main path of 
a query?
Is the new function better at dealing with bad input than the old function?

http://gerrit.cloudera.org:8080/#/c/14433/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14433/2//COMMIT_MSG@8
PS2, Line 8:
If you look at other Impala commit message you will see that they usually try 
to explain what is being done in the commit in some detail. I always imagine I 
am explaining what I am doing to some other developer. This is a place to 
discuss why as well as what you are doing, and any tradeoffs that were 
considered.

You should also add a section explaining what testing you did


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc
File be/src/util/auth-util.cc:

http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@90
PS2, Line 90:   krb5_context g_krb5_ctx;
Maybe 'krb5_ctx' is a clearer name?


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@91
PS2, Line 91:   CHECK_EQ(krb5_init_context(_krb5_ctx), 0);
Do you need unused_realm or to call krb5_get_default_realm?


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@93
PS2, Line 93:   CHECK_EQ(krb5_get_default_realm(g_krb5_ctx, _realm), 0);
What is the performance impact of these extra allocation/deallocations?


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@97
PS2, Line 97:   krb5_error_code code;
You could join with the next line:
krb5_error_code code = ...


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@99
PS2, Line 99:   if(code != 0) {
Is there a test that generates TErrorCode::BAD_PRINCIPAL_FORMAT?


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@100
PS2, Line 100: return Status(TErrorCode::BAD_PRINCIPAL_FORMAT, principal);
Maybe need to call
krb5_free_context()
before returning


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@105
PS2, Line 105:   char* p = data->data;
I think you can say
*service_name = data->data;


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@111
PS2, Line 111:   for(int j = 0; j < data->length; j++) {
*hostname = data->data;


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@114
PS2, Line 114:   krb5_free_principal(g_krb5_ctx, princ);
Maybe need to call
krb5_free_context()
before returning



-- 
To view, visit http://gerrit.cloudera.org:8080/14433
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e64ebdc10f102dbdc5b87f6fe3f2a0310b1be24
Gerrit-Change-Number: 14433
Gerrit-PatchSet: 2
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Tue, 15 Oct 2019 23:16:35 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7504/KUDU-2979 ParseKerberosPrincipal() should use krb5 parse name() instead

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14433 )

Change subject: IMPALA-7504/KUDU-2979 ParseKerberosPrincipal() should use 
krb5_parse_name() instead
..


Patch Set 7: Code-Review+2

LGTM, thank you


--
To view, visit http://gerrit.cloudera.org:8080/14433
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e64ebdc10f102dbdc5b87f6fe3f2a0310b1be24
Gerrit-Change-Number: 14433
Gerrit-PatchSet: 7
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Tue, 29 Oct 2019 21:00:15 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8065 OSInfo produces somewhat misleading output when running in container

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14531 )

Change subject: IMPALA-8065 OSInfo produces somewhat misleading output when 
running in container
..


Patch Set 2:

(7 comments)

Looks like this is getting close.

http://gerrit.cloudera.org:8080/#/c/14531/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14531/2//COMMIT_MSG@7
PS2, Line 7: IMPALA-8065 OSInfo produces somewhat misleading output when 
running in container
Sometimes it is better to have the first line be not the bug description but a 
oneline summary of the change.


http://gerrit.cloudera.org:8080/#/c/14531/2//COMMIT_MSG@9
PS2, Line 9: Original we get /proc/version dispalyed as OS version, while it's
Nit: displayed


http://gerrit.cloudera.org:8080/#/c/14531/2//COMMIT_MSG@14
PS2, Line 14: Tested locally, the displayed OS Info in localhost:25020 is:
"in localhost:25020" is confusing, maybe say on a Ubuntu 16 dev box?


http://gerrit.cloudera.org:8080/#/c/14531/2//COMMIT_MSG@21
PS2, Line 21: debian. Each OS picked one version to test.
Can you add a unit test where you check that os version and kernel version are 
not equal to "Unknown", then you will know this works on all tested platforms, 
and that it does not break on future platforms.


http://gerrit.cloudera.org:8080/#/c/14531/2/be/src/util/os-info.h
File be/src/util/os-info.h:

http://gerrit.cloudera.org:8080/#/c/14531/2/be/src/util/os-info.h@35
PS2, Line 35:   static const std::string os_version() {
Add a description of what this is


http://gerrit.cloudera.org:8080/#/c/14531/2/be/src/util/os-info.h@40
PS2, Line 40:   static const std::string kernel_version() {
Add a description of what this is


http://gerrit.cloudera.org:8080/#/c/14531/2/be/src/util/os-info.cc
File be/src/util/os-info.cc:

http://gerrit.cloudera.org:8080/#/c/14531/2/be/src/util/os-info.cc@58
PS2, Line 58:   string os_path_ = "Unknown";
local variables should not have the "_" suffux



--
To view, visit http://gerrit.cloudera.org:8080/14531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I848c9e53ee4e0bf8ae0874bb6da28e8efa7f7c8a
Gerrit-Change-Number: 14531
Gerrit-PatchSet: 2
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Tue, 29 Oct 2019 23:25:51 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7504/KUDU-2979 ParseKerberosPrincipal() should use krb5 parse name() instead

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14433 )

Change subject: IMPALA-7504/KUDU-2979 ParseKerberosPrincipal() should use 
krb5_parse_name() instead
..


Patch Set 6:

(3 comments)

Thanks for this update

http://gerrit.cloudera.org:8080/#/c/14433/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14433/5//COMMIT_MSG@21
PS5, Line 21: format principal, new error code is 2 instead of original 112
> What kind of end-to-end test? The one included in impala-private-parameteri
yes, maybe there's a better name but I think this name is generally understood


http://gerrit.cloudera.org:8080/#/c/14433/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14433/6//COMMIT_MSG@19
PS6, Line 19: Add two authentication-test
Add two tests to authentication-test


http://gerrit.cloudera.org:8080/#/c/14433/6/be/src/rpc/authentication-test.cc
File be/src/rpc/authentication-test.cc:

http://gerrit.cloudera.org:8080/#/c/14433/6/be/src/rpc/authentication-test.cc@200
PS6, Line 200:   EXPECT_ERROR(sa.InitKerberos(" ", "/etc/hosts"), 2);
This says we will get an error, but do we know it is the right error?
We should check the return code or message or something more specific



--
To view, visit http://gerrit.cloudera.org:8080/14433
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e64ebdc10f102dbdc5b87f6fe3f2a0310b1be24
Gerrit-Change-Number: 14433
Gerrit-PatchSet: 6
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Wed, 23 Oct 2019 17:03:19 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8571: harden QueryEventHook execution

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13748 )

Change subject: IMPALA-8571: harden QueryEventHook execution
..


Patch Set 28:

(25 comments)

Hi Radford. As Bharath and Fredy have disappeared I'll review this now :-)
Code looks good. Like you I'm slightly worried about the timeout queue. It's 
somehow wasteful to keep all that state around when the common case is that the 
tasks completed quickly.

http://gerrit.cloudera.org:8080/#/c/13748/28//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/13748/28//COMMIT_MSG@19
PS28, Line 19: See Java Thread.setDaemon(boolean) for what a "daemon thread" 
entails.
How can a user decide whether to use daemon threads?


http://gerrit.cloudera.org:8080/#/c/13748/28//COMMIT_MSG@24
PS28, Line 24: flag `query_event_hook_timeout_s`, which specified a timeout 
value
Nit: specifies


http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java
File 
fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java:

http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@47
PS28, Line 47:  * {@link QueryEventHook}s that utilizes a fixed thread pool 
pulling
fixed capacity thread pool?


http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@62
PS28, Line 62:  * submission, you will have no indication that rejection has 
taken place.
"there is no indication" i.e. passive voice may be clearer


http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@81
PS28, Line 81:  * For example, suppose you have 2 hook tasks that take roughly 
3 seconds to
This example is great, thanks


http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@82
PS28, Line 82:  * execute. You then create an executor with a thread pool of 
size 1 and a hook
You can consider this a nit :-) , but I find the use of 'you' jarring


http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@182
PS28, Line 182: final ArrayBlockingQueue boundedQueue =
Can you use BlockingQueue ?


http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@290
PS28, Line 290: t);
Does LOG.error work when there is an extra Throwable argument? Looks a bit 
weird to me


http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@315
PS28, Line 315: // a timeout-check task is scheduled for every hook task 
that is
Nit: capitalize 'a'


http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@318
PS28, Line 318: // task queue to grow unbounded if hook tasks are scheduled 
at an interval
This approach seems clever, and the code is neat and tidy...
So suppose I have problems getting my hooks to always execute. I know they 
usually take 5 seconds but just in case I will set the timeout to 30 mins. Now 
my timeout queue gets quite full I think?
How much garbage does the timeout queue keep from being collected?


http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@327
PS28, Line 327:   protected void checkHookTimeout(
private


http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@354
PS28, Line 354:   private static  CompletableFuture 
failedFuture(Throwable e) {
Maybe explain what is intended to happen


http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@365
PS28, Line 365:   private static String mname(String... names) {
Maybe mName is clearer?


http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@370
PS28, Line 370:   Counter getInterruptedCounter(QueryEventHook hook, String 
method) {
Unused?


http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/QueryEventHookManager.java
File fe/src/main/java/org/apache/impala/hooks/QueryEventHookManager.java:

http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/QueryEventHookManager.java@75
PS28, Line 75:   // constants to share with the executor so that no mismatches
Nit: Constants


http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/QueryEventHookManager.java@78
PS28, Line 78:   // it'd be nice if we could get the flag names from the be
Maybe instead say "The must be kept in sync with..."

[Impala-ASF-CR] IMPALA-8065 OSInfo produces somewhat misleading output when running in container

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14531 )

Change subject: IMPALA-8065 OSInfo produces somewhat misleading output when 
running in container
..


Patch Set 1:

(2 comments)

A couple of quick notes

http://gerrit.cloudera.org:8080/#/c/14531/1/be/src/util/os-info.cc
File be/src/util/os-info.cc:

http://gerrit.cloudera.org:8080/#/c/14531/1/be/src/util/os-info.cc@56
PS1, Line 56:   ifstream os_version("/etc/os-release", ios::in);
I think this file is not present on Centos6.
I *think* we still support that, though I am not 100% sure.


http://gerrit.cloudera.org:8080/#/c/14531/1/be/src/util/os-info.cc@99
PS1, Line 99:  << "Kernel version: " << kernel_version_ << endl
If I were reading this I would be confused: what is OS version, what is Kernel 
version? Are they the same?
If I am not in Docker what do I see?
My suggestion: only if we are running in Docker print
"Container OS: "



--
To view, visit http://gerrit.cloudera.org:8080/14531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I848c9e53ee4e0bf8ae0874bb6da28e8efa7f7c8a
Gerrit-Change-Number: 14531
Gerrit-PatchSet: 1
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 23 Oct 2019 01:00:28 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8998: admission control accounting for mt dop

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14357 )

Change subject: IMPALA-8998: admission control accounting for mt_dop
..


Patch Set 12:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/14357/12/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

http://gerrit.cloudera.org:8080/#/c/14357/12/be/src/scheduling/admission-controller.cc@247
PS12, Line 247: const string HOST_SLOT_NOT_AVAILABLE = "Not enough query slots 
available on host $0. "
I think we should remove all usages of the term 'query slot'.
The new terms 'admission control slot' and 'admission slot' are clearer, and we 
should use those. Ideally we might just use one of these two newer terms, but 
they are pretty clear as is.


http://gerrit.cloudera.org:8080/#/c/14357/12/be/src/scheduling/admission-controller.cc@573
PS12, Line 573:  << " admission_slots=" << admission_slots;
Nit: "executor admission_slots" might be more readable



--
To view, visit http://gerrit.cloudera.org:8080/14357
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b6b6262ef238df26b491352656a26e4163e46e5
Gerrit-Change-Number: 14357
Gerrit-PatchSet: 12
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 07 Oct 2019 17:00:25 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9034: fix distcc+ccache

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14408 )

Change subject: IMPALA-9034: fix distcc+ccache
..


Patch Set 1: Code-Review+2

This LGTM.
... when I took this code and ran with it I did not run my scripts that run
source "${IMPALA_HOME}"/bin/distcc/distcc_env.sh
and somehow when I ran buildall I got in the situation where distcc thinks it 
can spawn many threads but they all ran locally. Which locks up the machine.
Now I run my scripts correctly and all is good.


--
To view, visit http://gerrit.cloudera.org:8080/14408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0b080709bd765056b9296d3deb805038fc01e5d
Gerrit-Change-Number: 14408
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Fri, 11 Oct 2019 16:44:35 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9240: add HTTP code handling to THttpClient.

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14924 )

Change subject: IMPALA-9240: add HTTP code handling to THttpClient.
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14924/2/tests/shell/test_shell_interactive.py
File tests/shell/test_shell_interactive.py:

http://gerrit.cloudera.org:8080/#/c/14924/2/tests/shell/test_shell_interactive.py@20
PS2, Line 20: import httplib
> nit: whitespace above this
Done



--
To view, visit http://gerrit.cloudera.org:8080/14924
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3c105f4b8237b87695324d75981821c08c43
Gerrit-Change-Number: 14924
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Thu, 19 Dec 2019 19:41:32 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9240: add HTTP code handling to THttpClient.

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14924 )

Change subject: IMPALA-9240: add HTTP code handling to THttpClient.
..


Patch Set 3: Code-Review+2

pull forward +2


--
To view, visit http://gerrit.cloudera.org:8080/14924
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3c105f4b8237b87695324d75981821c08c43
Gerrit-Change-Number: 14924
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Thu, 19 Dec 2019 19:42:44 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9240: add HTTP code handling to THttpClient.

[Andrew Sherman (Code Review)]

Andrew Sherman has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/14924 )

Change subject: IMPALA-9240: add HTTP code handling to THttpClient.
..

IMPALA-9240: add HTTP code handling to THttpClient.

Before this change Impala Shell is not checking HTTP return codes when
using the hs2-http protocol. The shell is sending a request message
(e.g. send_CloseOperation) but the HTTP call to send this message may
fail. This will result in a failure when reading the reply (e.g. in
recv_CloseOperation) as there is no reply data to read. This will
typically result in an 'EOFError'.

In code that overrides THttpClient.flush(), check the HTTP code that is
returned after the HTTP call is made. If the code is not 1XX
(informational response) or 2XX (successful) then throw an RPCException.

This change does not contain any attempt to recover from an HTTP failures
but it does allow the failure to be detected and a message to be
printed.

In future it may be possible to retry after certain HTTP errors.

Testing:
- Add a new test for impala-shell that tries to connect to an HTTP
  server that always returns a 503 error. Check that an appropriate
  error message is printed.

Change-Id: I3c105f4b8237b87695324d75981821c08c43
---
M shell/impala_client.py
M tests/shell/test_shell_interactive.py
2 files changed, 62 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/14924/3
--
To view, visit http://gerrit.cloudera.org:8080/14924
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3c105f4b8237b87695324d75981821c08c43
Gerrit-Change-Number: 14924
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-9240: add HTTP code handling to THttpClient.

[Andrew Sherman (Code Review)]

Andrew Sherman has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/14924


Change subject: IMPALA-9240: add HTTP code handling to THttpClient.
..

IMPALA-9240: add HTTP code handling to THttpClient.

Before this change Impala Shell is not checking HTTP return codes when
using the hs2-http protocol. The shell is sending a request message
(e.g. send_CloseOperation) but the HTTP call to send this message may
fail. This will result in a failure when reading the reply (e.g. in
recv_CloseOperation) as there is no reply data to read. This will
typically result in an 'EOFError'.

In THttpClient.flush() check the HTTP code that is returned after the
HTTP call is made. If the code is not 1XX (informational response) or
2XX (successful) then throw an RPCException.

This change does not contain any attempt to recover from an HTTP failures
but it does allow the failure to be detected and a message to be
printed.

In future it may be possible to retry after certain HTTP errors.

Testing:
- Add a new test for impala-shell that tries to connect to an HTTP
  server that always returns a 503 error. Check that an appropriate
  error message is printed.

Change-Id: I3c105f4b8237b87695324d75981821c08c43
---
M shell/impala_client.py
M tests/shell/test_shell_interactive.py
2 files changed, 62 insertions(+), 4 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/14924/1
--
To view, visit http://gerrit.cloudera.org:8080/14924
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3c105f4b8237b87695324d75981821c08c43
Gerrit-Change-Number: 14924
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-9240: add HTTP code handling to THttpClient.

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14924 )

Change subject: IMPALA-9240: add HTTP code handling to THttpClient.
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14924/1/tests/shell/test_shell_interactive.py
File tests/shell/test_shell_interactive.py:

http://gerrit.cloudera.org:8080/#/c/14924/1/tests/shell/test_shell_interactive.py@868
PS1, Line 868:   return
> I feel like this should be pytest.skip(). I think using return here will me
Thanks for the pythonic suggestion



--
To view, visit http://gerrit.cloudera.org:8080/14924
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3c105f4b8237b87695324d75981821c08c43
Gerrit-Change-Number: 14924
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Wed, 18 Dec 2019 22:28:48 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9240: add HTTP code handling to THttpClient.

[Andrew Sherman (Code Review)]

Andrew Sherman has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/14924 )

Change subject: IMPALA-9240: add HTTP code handling to THttpClient.
..

IMPALA-9240: add HTTP code handling to THttpClient.

Before this change Impala Shell is not checking HTTP return codes when
using the hs2-http protocol. The shell is sending a request message
(e.g. send_CloseOperation) but the HTTP call to send this message may
fail. This will result in a failure when reading the reply (e.g. in
recv_CloseOperation) as there is no reply data to read. This will
typically result in an 'EOFError'.

In code that overrides THttpClient.flush(), check the HTTP code that is
returned after the HTTP call is made. If the code is not 1XX
(informational response) or 2XX (successful) then throw an RPCException.

This change does not contain any attempt to recover from an HTTP failures
but it does allow the failure to be detected and a message to be
printed.

In future it may be possible to retry after certain HTTP errors.

Testing:
- Add a new test for impala-shell that tries to connect to an HTTP
  server that always returns a 503 error. Check that an appropriate
  error message is printed.

Change-Id: I3c105f4b8237b87695324d75981821c08c43
---
M shell/impala_client.py
M tests/shell/test_shell_interactive.py
2 files changed, 62 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/14924/2
--
To view, visit http://gerrit.cloudera.org:8080/14924
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3c105f4b8237b87695324d75981821c08c43
Gerrit-Change-Number: 14924
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-9384: Improve Impala shell usability by automatically enable live progress in the interactive mode

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15219 )

Change subject: IMPALA-9384: Improve Impala shell usability by automatically 
enable live_progress in the interactive mode
..


Patch Set 1:

(8 comments)

This looks like a good change, thanks.
I have a few nits and questions.

http://gerrit.cloudera.org:8080/#/c/15219/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/15219/1//COMMIT_MSG@8
PS1, Line 8: live_progress in the interactive mode
I think the single line starting with 'IMPALA-9384' should be a short single 
line summary, so it should not overflow onto the next line.


http://gerrit.cloudera.org:8080/#/c/15219/1//COMMIT_MSG@12
PS1, Line 12: mode when users do not set the option in both configure file and 
command
Should this be "in either the configuration file or as a command line flag"?


http://gerrit.cloudera.org:8080/#/c/15219/1//COMMIT_MSG@19
PS1, Line 19:
Add a documentation jira to update docs for the change, or refer to it in the 
commit message if you already added it.


http://gerrit.cloudera.org:8080/#/c/15219/1/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/15219/1/shell/impala_shell.py@1728
PS1, Line 1728:   # config_file_live_progress is None, which means user didn't 
set live_progress
Nit: maybe clearer to say "Set config_file_live_progress to None" ...


http://gerrit.cloudera.org:8080/#/c/15219/1/shell/impala_shell.py@1742
PS1, Line 1742:   config_file_live_progress = s_options.get('live_progress')
As this is being set in a loop, is it possible that the value can be set to 
something the first time though, and then overwritten back to None the next 
time through?


http://gerrit.cloudera.org:8080/#/c/15219/1/shell/impala_shell.py@1753
PS1, Line 1753:   # live_progress in the config file, Impala shell will 
automatically enable it
Nit: end with a period


http://gerrit.cloudera.org:8080/#/c/15219/1/shell/impala_shell.py@1754
PS1, Line 1754:   if not options.query and not options.query_file and 
config_file_live_progress is None:
Can we simplify by using 'self.interactive' here?


http://gerrit.cloudera.org:8080/#/c/15219/1/tests/shell/test_shell_interactive.py
File tests/shell/test_shell_interactive.py:

http://gerrit.cloudera.org:8080/#/c/15219/1/tests/shell/test_shell_interactive.py@503
PS1, Line 503: # file and command line, Impala shell will automatically 
enable it
Nit: end comment with a period



--
To view, visit http://gerrit.cloudera.org:8080/15219
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3765b775f663fa227e59728acffe4d5ea9a5e2d3
Gerrit-Change-Number: 15219
Gerrit-PatchSet: 1
Gerrit-Owner: Alice Fan 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 24 Feb 2020 19:02:32 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9075: Add support for reading zstd text files

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15023 )

Change subject: IMPALA-9075: Add support for reading zstd text files
..


Patch Set 8: Code-Review+2

LGTM thanks Xiaomeng and Abhishek


--
To view, visit http://gerrit.cloudera.org:8080/15023
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2adce9fe00190558525fa5cd3d50cf5e0f0b0aa4
Gerrit-Change-Number: 15023
Gerrit-PatchSet: 8
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Mon, 24 Feb 2020 19:49:49 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8852: Skip short-circuit config check for dedicated coordinator

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15173 )

Change subject: IMPALA-8852: Skip short-circuit config check for dedicated 
coordinator
..


Patch Set 10: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/15173
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I373d4037f4cee203322a398b77b75810ba708bb5
Gerrit-Change-Number: 15173
Gerrit-PatchSet: 10
Gerrit-Owner: Tamas Mate 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Anurag Mantripragada 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tamas Mate 
Gerrit-Comment-Date: Mon, 24 Feb 2020 20:02:07 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9389: [DOCS] Support reading zstd text files

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15304 )

Change subject: IMPALA-9389: [DOCS] Support reading zstd text files
..


Patch Set 6: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/15304
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic83137bd2c3a49398fb60cf1901f8b74ed111fce
Gerrit-Change-Number: 15304
Gerrit-PatchSet: 6
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Fri, 28 Feb 2020 22:35:59 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9384: Improve Impala shell usability by automatically enable live progress in the interactive mode

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15219 )

Change subject: IMPALA-9384: Improve Impala shell usability by automatically 
enable live_progress in the interactive mode
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/15219/1/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/15219/1/shell/impala_shell.py@1742
PS1, Line 1742:   config_file_live_progress = s_options.get('live_progress')
> I think I didn't get what you mean here. can you please elaborate? thanks
Line 1731 set config_file_live_progress = None
Line 1737 puts us in a loop through config files
- Say first config file has a value for 'live_progress', now on Line 1742 
config_file_live_progress=True
- Say second config file does not have a value for  'live_progress', now on 
Line 1742 config_file_live_progress=None again

So config_file_live_progress gets set then overwritten, that seems weird.
(Probably this doesn't matter if you rewrite as dknupp suggests)


http://gerrit.cloudera.org:8080/#/c/15219/1/shell/impala_shell.py@1754
PS1, Line 1754:   if not options.query and not options.query_file and 
config_file_live_progress is None:
> I think we are doing the if statement at main() and the self.interactive is
OK



--
To view, visit http://gerrit.cloudera.org:8080/15219
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3765b775f663fa227e59728acffe4d5ea9a5e2d3
Gerrit-Change-Number: 15219
Gerrit-PatchSet: 1
Gerrit-Owner: Alice Fan 
Gerrit-Reviewer: Alice Fan 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 28 Feb 2020 22:20:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9431 [DOCS] Update docs to reflect Deflate is supported for text files

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15310 )

Change subject: IMPALA-9431 [DOCS] Update docs to reflect Deflate is supported 
for text files
..


Patch Set 3:

(1 comment)

A quick comment on the commit message, otherwise this looks good to me.

http://gerrit.cloudera.org:8080/#/c/15310/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/15310/3//COMMIT_MSG@11
PS3, Line 11: - 123, 263, 634, 646, 658, 700, 701, 710
Nit: I don't think this level of detail is helpful in the commit message. 
Anyone who cares can see this detail in gerrit. Better I think to have a 
general description of the change.



--
To view, visit http://gerrit.cloudera.org:8080/15310
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e1205e4e408f2c20fd8642cccd6c74e7ba9eb40
Gerrit-Change-Number: 15310
Gerrit-PatchSet: 3
Gerrit-Owner: Kristine Hahn 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Thu, 05 Mar 2020 01:36:22 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9384: Improve Impala shell usability by enabling live progress in interactive mode

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15219 )

Change subject: IMPALA-9384: Improve Impala shell usability by enabling 
live_progress in interactive mode
..


Patch Set 3:

(3 comments)

I have nits about commit message only, will let David look at code

http://gerrit.cloudera.org:8080/#/c/15219/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/15219/3//COMMIT_MSG@9
PS3, Line 9: In order to improve usability, this patch would like to make 
Impala shell show query
The commit message describes the change. So "this patch makes Impala Shell show 
..."


http://gerrit.cloudera.org:8080/#/c/15219/3//COMMIT_MSG@11
PS3, Line 11: live_progress by default when a user launches impala shell in the 
interactive mode.
Nit: in general, unless you have some special text, you should word wrap at 72 
chars in a commit message


http://gerrit.cloudera.org:8080/#/c/15219/3//COMMIT_MSG@14
PS3, Line 14: live_progress by either using the proposed command line flag or 
setting the option as
Don't say 'proposed' the commit message describes what changed.



--
To view, visit http://gerrit.cloudera.org:8080/15219
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3765b775f663fa227e59728acffe4d5ea9a5e2d3
Gerrit-Change-Number: 15219
Gerrit-PatchSet: 3
Gerrit-Owner: Alice Fan 
Gerrit-Reviewer: Alice Fan 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 03 Mar 2020 18:05:09 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9431 [DOCS] Update docs to reflect Deflate is supported for text files

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15310 )

Change subject: IMPALA-9431 [DOCS] Update docs to reflect Deflate is supported 
for text files
..


Patch Set 5: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/15310
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e1205e4e408f2c20fd8642cccd6c74e7ba9eb40
Gerrit-Change-Number: 15310
Gerrit-PatchSet: 5
Gerrit-Owner: Kristine Hahn 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Fri, 06 Mar 2020 17:28:23 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9389: [DOCS] Support reading zstd text files

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15304 )

Change subject: IMPALA-9389: [DOCS] Support reading zstd text files
..


Patch Set 2:

(1 comment)

I think (Abhishek and Xiaomeng to confirm) that zstd is most easily explained 
as being another form of text compression, like gzip, bzip2, or Snappy.

http://gerrit.cloudera.org:8080/#/c/15304/2/docs/topics/impala_txtfile.xml
File docs/topics/impala_txtfile.xml:

http://gerrit.cloudera.org:8080/#/c/15304/2/docs/topics/impala_txtfile.xml@644
PS2, Line 644: Using gzip, bzip2, or Snappy-Compressed Text 
Files
Maybe this is the right place to add doc for zstd?
I think zstd is like these other formats.



--
To view, visit http://gerrit.cloudera.org:8080/15304
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic83137bd2c3a49398fb60cf1901f8b74ed111fce
Gerrit-Change-Number: 15304
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Thu, 27 Feb 2020 17:32:47 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9075: Add support for reading zstd text files

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15023 )

Change subject: IMPALA-9075: Add support for reading zstd text files
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15023/4/be/src/util/decompress.cc
File be/src/util/decompress.cc:

http://gerrit.cloudera.org:8080/#/c/15023/4/be/src/util/decompress.cc@665
PS4, Line 665: *stream_end = false;
> The reason I add it here is that other stream decompressing functions write
I would have written the code the same way!
But, now the scary comment has been removed, we can see that the loop is really 
quite simple. If we remove another line then it is even simpler, which I like.
Clearly the extra setting into *stream_end is safe, so it's not "wrong" but 
anything that makes things clearer for the future maintainer is good.
If you feel strongly then we can talk more



--
To view, visit http://gerrit.cloudera.org:8080/15023
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2adce9fe00190558525fa5cd3d50cf5e0f0b0aa4
Gerrit-Change-Number: 15023
Gerrit-PatchSet: 4
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Thu, 30 Jan 2020 00:28:51 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8852: Skipping short-circuit config check for coordinator only

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15173 )

Change subject: IMPALA-8852: Skipping short-circuit config check for 
coordinator only
..


Patch Set 2:

(5 comments)

Thanks for the change. This looks good.
In the Jira Lars says "As a permanent solution we should only emit a warning 
when the socket cannot be found and -is_executor=false". This solution is 
different in that it just avoids the check. That is probably OK but can you 
explain why you're not doing what Lars said.

http://gerrit.cloudera.org:8080/#/c/15173/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/15173/2//COMMIT_MSG@10
PS2, Line 10: DataNode is not avaiable on the hosts. This change adds a 
condition to
Nit: typo: available


http://gerrit.cloudera.org:8080/#/c/15173/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:

http://gerrit.cloudera.org:8080/#/c/15173/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@723
PS2, Line 723: if (!BackendConfig.INSTANCE.getBackendCfg().is_executor) {
Should this check move after the test for DFS_CLIENT_READ_SHORTCIRCUIT_KEY?
If short-circuit reads are not enabled then this message might be confusing?


http://gerrit.cloudera.org:8080/#/c/15173/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@724
PS2, Line 724:   LOG.info("Coordinator only instance will not read local 
data via " +
Nit "Coordinator-only" is clearer I think


http://gerrit.cloudera.org:8080/#/c/15173/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@725
PS2, Line 725:   "short-circuit reads.");
We use clang-format to format Java code (as well as C++)
https://cwiki.apache.org/confluence/display/IMPALA/Impala+Style+Guide
You can get a plugin for IntelliJ which makes it easy to do.
It finds a few small nits in your code layout.


http://gerrit.cloudera.org:8080/#/c/15173/2/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java
File fe/src/test/java/org/apache/impala/service/JniFrontendTest.java:

http://gerrit.cloudera.org:8080/#/c/15173/2/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@68
PS2, Line 68: // GIVEN
I wasn't familiar with this given-when-then style but I googled it and learned 
something. As I'm not used to it I found it a little jarring as this is the 
only use of it I have seen in the Impala codebase



--
To view, visit http://gerrit.cloudera.org:8080/15173
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I373d4037f4cee203322a398b77b75810ba708bb5
Gerrit-Change-Number: 15173
Gerrit-PatchSet: 2
Gerrit-Owner: Tamas Mate 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Anurag Mantripragada 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Thu, 06 Feb 2020 18:55:23 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9075: Add support for reading zstd text files

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15023 )

Change subject: IMPALA-9075: Add support for reading zstd text files
..


Patch Set 5: Code-Review+1

LGTM, @arawat will review to +2


--
To view, visit http://gerrit.cloudera.org:8080/15023
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2adce9fe00190558525fa5cd3d50cf5e0f0b0aa4
Gerrit-Change-Number: 15023
Gerrit-PatchSet: 5
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Fri, 31 Jan 2020 19:23:45 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8852: Skip short-circuit config check for coordinator-only mode

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15173 )

Change subject: IMPALA-8852: Skip short-circuit config check for 
coordinator-only mode
..


Patch Set 3: Code-Review+1

LGTM
Giving +1 for now, I can +2/commit once Anurag is happy


--
To view, visit http://gerrit.cloudera.org:8080/15173
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I373d4037f4cee203322a398b77b75810ba708bb5
Gerrit-Change-Number: 15173
Gerrit-PatchSet: 3
Gerrit-Owner: Tamas Mate 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Anurag Mantripragada 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tamas Mate 
Gerrit-Comment-Date: Sat, 08 Feb 2020 01:36:11 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9075: Add support for reading zstd text files

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15023 )

Change subject: IMPALA-9075: Add support for reading zstd text files
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15023/4/be/src/util/decompress.cc
File be/src/util/decompress.cc:

http://gerrit.cloudera.org:8080/#/c/15023/4/be/src/util/decompress.cc@665
PS4, Line 665: *stream_end = false;
It seems to me that this line is redundant.
*stream_end = true
is only set if (input_buffer.pos == input_buffer.size)
in which case we will break out of the loop.



--
To view, visit http://gerrit.cloudera.org:8080/15023
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2adce9fe00190558525fa5cd3d50cf5e0f0b0aa4
Gerrit-Change-Number: 15023
Gerrit-PatchSet: 4
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Wed, 29 Jan 2020 00:30:41 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8852: Skip short-circuit config check for coordinator-only mode

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15173 )

Change subject: IMPALA-8852: Skip short-circuit config check for 
coordinator-only mode
..


Patch Set 4: Code-Review+2

LGTM, thanks


--
To view, visit http://gerrit.cloudera.org:8080/15173
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I373d4037f4cee203322a398b77b75810ba708bb5
Gerrit-Change-Number: 15173
Gerrit-PatchSet: 4
Gerrit-Owner: Tamas Mate 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Anurag Mantripragada 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tamas Mate 
Gerrit-Comment-Date: Mon, 10 Feb 2020 17:28:27 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9274: cyclic barrier implementation

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14973 )

Change subject: IMPALA-9274: cyclic barrier implementation
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14973/3/be/src/util/cyclic-barrier.h
File be/src/util/cyclic-barrier.h:

http://gerrit.cloudera.org:8080/#/c/14973/3/be/src/util/cyclic-barrier.h@71
PS3, Line 71:   // The number of threads partipating in synchronization.
Nit: participating



--
To view, visit http://gerrit.cloudera.org:8080/14973
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id5c8cc6b3023d2dc03c8116d462440c5e25c3bb0
Gerrit-Change-Number: 14973
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 03 Jan 2020 17:54:49 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9241: Remove pid files on successful shutdown of minicluster

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14950 )

Change subject: IMPALA-9241: Remove pid files on successful shutdown of 
minicluster
..


Patch Set 1: Code-Review+2

LGTM


--
To view, visit http://gerrit.cloudera.org:8080/14950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b14d74df8061b6595b9897df9c9667e3f569e34
Gerrit-Change-Number: 14950
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Fri, 27 Dec 2019 18:05:29 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9666 Correct a log message in TestImpalaShellInteractive

[Andrew Sherman (Code Review)]

Andrew Sherman has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/15751


Change subject: IMPALA-9666 Correct a log message in TestImpalaShellInteractive
..

IMPALA-9666 Correct a log message in TestImpalaShellInteractive

When _wait_for_num_open_sessions() calls wait_for_metric_value() and
wait_for_metric_value() hits an assertion error then
_wait_for_num_open_sessions() catches the exception and attempts to log
a descriptive string that was passed to it as a parameter. The logging
call does not have a '%s' directive, which results in the failure being
reported as
 "TypeError: not all arguments converted during string formatting".
Fix this by adding the '%s' directive to the logging call.

Change-Id: Icfd7baed153dadceb953df30beba180128055c92
---
M tests/shell/test_shell_interactive.py
1 file changed, 1 insertion(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/15751/1
--
To view, visit http://gerrit.cloudera.org:8080/15751
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icfd7baed153dadceb953df30beba180128055c92
Gerrit-Change-Number: 15751
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman 

[Impala-ASF-CR] IMPALA-9540 Test that Impala Shell no longer sends duplicate "Host" headers in http mode.

[Andrew Sherman (Code Review)]

Andrew Sherman has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/15752


Change subject: IMPALA-9540 Test that Impala Shell no longer sends duplicate 
"Host" headers in http mode.
..

IMPALA-9540 Test that Impala Shell no longer sends duplicate "Host" headers in 
http mode.

Many http servers will not accept an http request that has multiple
copies of the "Host" header. A recent toolchain change patches
Thrift so that will not send the extraneous header (in THttpClient).
This change tests that the duplicate headers are not sent,

TESTING:
  Ran all end-to-end tests.
  Enhanced an existing Shell test to check that only one "Host" header
  is sent.

Change-Id: I82996015d0205923e854dac8bb88604778684c46
---
M tests/shell/test_shell_interactive.py
1 file changed, 23 insertions(+), 7 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/15752/1
--
To view, visit http://gerrit.cloudera.org:8080/15752
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I82996015d0205923e854dac8bb88604778684c46
Gerrit-Change-Number: 15752
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: David Knupp 

[Impala-ASF-CR] IMPALA-9665: Fixed database not found errors in query test.test insert

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15766 )

Change subject: IMPALA-9665: Fixed database not found errors in 
query_test.test_insert
..


Patch Set 1:

Looks good, will wait for build to run.
Gerrit says "Cannot Merge" so you may need to rebase.


--
To view, visit http://gerrit.cloudera.org:8080/15766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b7aa3775dd4375f536d76f2e236ce126f8c78cd
Gerrit-Change-Number: 15766
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Tamas 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Mon, 20 Apr 2020 22:51:08 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9665: Fixed database not found errors in query test.test insert

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15766 )

Change subject: IMPALA-9665: Fixed database not found errors in 
query_test.test_insert
..


Patch Set 2: Code-Review+2

LGTM


--
To view, visit http://gerrit.cloudera.org:8080/15766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b7aa3775dd4375f536d76f2e236ce126f8c78cd
Gerrit-Change-Number: 15766
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Tamas 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Tue, 21 Apr 2020 01:51:09 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9483 Add logs for debugging builtin functions throw unknown exception randomly

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15607 )

Change subject: IMPALA-9483 Add logs for debugging builtin functions throw 
unknown exception randomly
..


Patch Set 2:

(2 comments)

I have a couple of small linguistic suggestions, otherwise this looks good.

http://gerrit.cloudera.org:8080/#/c/15607/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/15607/2//COMMIT_MSG@10
PS2, Line 10: randomly fails with function unknown error. For example,
Nit: randomly fail when trying to find the function. For example,"


http://gerrit.cloudera.org:8080/#/c/15607/2/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:

http://gerrit.cloudera.org:8080/#/c/15607/2/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@494
PS2, Line 494:   + ". Currently this db has function number " + 
db.numFunctions());
Slighly clearer:
". Currently this db has " + db.numFunctions() + " functions.");



--
To view, visit http://gerrit.cloudera.org:8080/15607
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I30d6eb697695da8d2521acb76d8310ec8f1bbda9
Gerrit-Change-Number: 15607
Gerrit-PatchSet: 2
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Tue, 31 Mar 2020 00:57:40 +
Gerrit-HasComments: Yes

[native-toolchain-CR] Patch Thrift to 0.9.3-p8 to remove a duplicate 'Host' header in http messages.

[Andrew Sherman (Code Review)]

Andrew Sherman has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/15620


Change subject: Patch Thrift to 0.9.3-p8 to remove a duplicate 'Host' header in 
http messages.
..

Patch Thrift to 0.9.3-p8 to remove a duplicate 'Host' header in http messages.

This will be used to fix IMPALA-9540 "Impala Shell sends duplicate
"Host" headers in http mode". Many http servers will not accept an http
request that has multiple copies of the Host header. Remove the
extraneous call which adds an unnecessary header.

TESTING:

I built the toolchain using jenkins. I built impala with the new
toolchain, ran all Impala end-to-end tests, and tested that IMPALA-9540
is fixed.

Change-Id: If5fbeeba4c55423128733ec15a3dbabf2b6ac8a6
---
M buildall.sh
A source/thrift/thrift-0.9.3-patches/0008-remove-duplicated-host-header.patch
2 files changed, 28 insertions(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/native-toolchain 
refs/changes/20/15620/1
--
To view, visit http://gerrit.cloudera.org:8080/15620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If5fbeeba4c55423128733ec15a3dbabf2b6ac8a6
Gerrit-Change-Number: 15620
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman 

[Impala-ASF-CR] IMPALA-9483 Add logs for debugging builtin functions throw unknown exception randomly

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15607 )

Change subject: IMPALA-9483 Add logs for debugging builtin functions throw 
unknown exception randomly
..


Patch Set 3: Code-Review+2

LGTM, thanks


--
To view, visit http://gerrit.cloudera.org:8080/15607
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I30d6eb697695da8d2521acb76d8310ec8f1bbda9
Gerrit-Change-Number: 15607
Gerrit-PatchSet: 3
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Wed, 01 Apr 2020 15:26:20 +
Gerrit-HasComments: No

[native-toolchain-CR] Patch Thrift to 0.9.3-p8 to remove a duplicate 'Host' header in http messages.

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15620 )

Change subject: Patch Thrift to 0.9.3-p8 to remove a duplicate 'Host' header in 
http messages.
..


Patch Set 1: Verified+1


--
To view, visit http://gerrit.cloudera.org:8080/15620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If5fbeeba4c55423128733ec15a3dbabf2b6ac8a6
Gerrit-Change-Number: 15620
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 01 Apr 2020 19:20:53 +
Gerrit-HasComments: No

[native-toolchain-CR] Patch Thrift to 0.9.3-p8 to remove a duplicate 'Host' header in http messages.

[Andrew Sherman (Code Review)]

Andrew Sherman has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/15620 )

Change subject: Patch Thrift to 0.9.3-p8 to remove a duplicate 'Host' header in 
http messages.
..

Patch Thrift to 0.9.3-p8 to remove a duplicate 'Host' header in http messages.

This will be used to fix IMPALA-9540 "Impala Shell sends duplicate
"Host" headers in http mode". Many http servers will not accept an http
request that has multiple copies of the Host header. Remove the
extraneous call which adds an unnecessary header.

TESTING:

I built the toolchain using jenkins. I built impala with the new
toolchain, ran all Impala end-to-end tests, and tested that IMPALA-9540
is fixed.

Change-Id: If5fbeeba4c55423128733ec15a3dbabf2b6ac8a6
Reviewed-on: http://gerrit.cloudera.org:8080/15620
Reviewed-by: Tim Armstrong 
Tested-by: Andrew Sherman 
---
M buildall.sh
A source/thrift/thrift-0.9.3-patches/0008-remove-duplicated-host-header.patch
2 files changed, 28 insertions(+), 1 deletion(-)

Approvals:
  Tim Armstrong: Looks good to me, approved
  Andrew Sherman: Verified

--
To view, visit http://gerrit.cloudera.org:8080/15620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If5fbeeba4c55423128733ec15a3dbabf2b6ac8a6
Gerrit-Change-Number: 15620
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-9681: Fix LdapImpalaShellTest

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15777 )

Change subject: IMPALA-9681: Fix LdapImpalaShellTest
..


Patch Set 1: Code-Review+2

Thanks for fixing this


--
To view, visit http://gerrit.cloudera.org:8080/15777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e7070fa6da4085ea858f64141529a7a23a86534
Gerrit-Change-Number: 15777
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Tue, 21 Apr 2020 21:50:34 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9574: support ubuntu 18.04 base image

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15765 )

Change subject: IMPALA-9574: support ubuntu 18.04 base image
..


Patch Set 2: Code-Review+2

(1 comment)

LGTM

http://gerrit.cloudera.org:8080/#/c/15765/2/docker/impala_base/Dockerfile
File docker/impala_base/Dockerfile:

http://gerrit.cloudera.org:8080/#/c/15765/2/docker/impala_base/Dockerfile@17
PS2, Line 17: ARG BASE_IMAGE=ubuntu:16.04
Can this default value ever be used now that we expect BASE_IMAGE to be always 
set as in the docker invocation?



--
To view, visit http://gerrit.cloudera.org:8080/15765
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8dfdb349e78fd76b91138a70449d51b0ef0021df
Gerrit-Change-Number: 15765
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Anurag Mantripragada 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 21 Apr 2020 23:51:15 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9540 Test that Impala Shell no longer sends duplicate "Host" headers in http mode.

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15752 )

Change subject: IMPALA-9540 Test that Impala Shell no longer sends duplicate 
"Host" headers in http mode.
..


Patch Set 1:

(1 comment)

Let me know if you have more questions about this slightly tricky code.

http://gerrit.cloudera.org:8080/#/c/15752/1/tests/shell/test_shell_interactive.py
File tests/shell/test_shell_interactive.py:

http://gerrit.cloudera.org:8080/#/c/15752/1/tests/shell/test_shell_interactive.py@88
PS1, Line 88: self.headers.headers
> Sorry for this potentially obstuse question, but I have to confess, I'm a l
OK headers.headers does look weird.
Here self.headers is an attribute of the SimpleHTTPRequestHandler (actually the 
base class BaseHTTPServer.BaseHTTPRequestHandler). It's type is MeeasageClass 
which inherits from rfc822.Message. This type also has a headers attribute 
which is a simple list of strings.
The TestRequestHandler.headers is a class attribute which I am using as a rock 
to put the saved headers under. This is hacky and would not work in real code 
as that class attribute would be shared by all threads.
I am doing all this so I can save the headers that are received on the server 
side of the http call.



--
To view, visit http://gerrit.cloudera.org:8080/15752
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82996015d0205923e854dac8bb88604778684c46
Gerrit-Change-Number: 15752
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Wed, 22 Apr 2020 18:19:54 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9540 Test that Impala Shell no longer sends duplicate "Host" headers in http mode.

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15752 )

Change subject: IMPALA-9540 Test that Impala Shell no longer sends duplicate 
"Host" headers in http mode.
..


Patch Set 1:

(1 comment)

I have pretty much copied what you suggested :-)

http://gerrit.cloudera.org:8080/#/c/15752/1/tests/shell/test_shell_interactive.py
File tests/shell/test_shell_interactive.py:

http://gerrit.cloudera.org:8080/#/c/15752/1/tests/shell/test_shell_interactive.py@88
PS1, Line 88: self.headers.headers
> Sorry about the line wrapping.
Thanks for the really useful review.
The thing I had not understood was how useful it is to use a
@pytest.yield_fixture
even for a one-use situation.
And they don't have to be in conftest.py



--
To view, visit http://gerrit.cloudera.org:8080/15752
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82996015d0205923e854dac8bb88604778684c46
Gerrit-Change-Number: 15752
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Tue, 05 May 2020 23:38:01 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9540 Test that Impala Shell no longer sends duplicate "Host" headers in http mode.

[Andrew Sherman (Code Review)]

Andrew Sherman has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/15752 )

Change subject: IMPALA-9540 Test that Impala Shell no longer sends duplicate 
"Host" headers in http mode.
..

IMPALA-9540 Test that Impala Shell no longer sends duplicate "Host" headers in 
http mode.

Many http servers will not accept an http request that has multiple
copies of the "Host" header. A recent toolchain change patches
Thrift so that will not send the extraneous header (in THttpClient).
This change tests that the duplicate headers are not sent,

TESTING:
  Ran all end-to-end tests.
  Rewrote an existing Shell test to check that only one "Host" header
  is sent.

Change-Id: I82996015d0205923e854dac8bb88604778684c46
---
M tests/shell/test_shell_interactive.py
M tests/shell/util.py
2 files changed, 53 insertions(+), 34 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/15752/2
--
To view, visit http://gerrit.cloudera.org:8080/15752
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I82996015d0205923e854dac8bb88604778684c46
Gerrit-Change-Number: 15752
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins 

[Impala-ASF-CR] IMPALA-9743: Fix ExprCardinalityTest may not be marked as in test env

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15982 )

Change subject: IMPALA-9743: Fix ExprCardinalityTest may not be marked as in 
test env
..


Patch Set 1: Code-Review+2

LGTM


--
To view, visit http://gerrit.cloudera.org:8080/15982
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5806dea77eb8e10ccf8ac749b7256603bd79cd97
Gerrit-Change-Number: 15982
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Sat, 23 May 2020 15:41:34 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10244: Make non-scalable failures to dequeue observable.

[Andrew Sherman (Code Review)]

Andrew Sherman has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/16613


Change subject: IMPALA-10244: Make non-scalable failures to dequeue observable.
..

IMPALA-10244: Make non-scalable failures to dequeue observable.

One of the important ways to observe Impala throughput is by looking at
when queries are queued. This can be an indication that more resources
should be added to the cluster by adding more executor groups. This is
only a good strategy if adding more resources will help with the current
workload. In some situations the head of the query queue cannot be
executed because of resource constraints on the coordinator. In these
cases the coordinator is the bottleneck so adding more executor groups
will not help. This change is to make these cases observable by adding a
new counter which is incremented when a dequeue fails because of
resource constraints that would not be resolved by adding more executor
groups.

The two cases that cause the counter to be incremented are:
- when there are not enough admission control slots on the coordinator
- when there is not enough memory on the coordinator
but it is possible that other conditions may be added in future.

TESTING:
  Added new unit tests.
  Ran all end-to-end tests.

Change-Id: I3456396ac139c562ad9cd3ac1a624d8f35487518
---
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M common/thrift/metrics.json
M tests/custom_cluster/test_admission_controller.py
5 files changed, 157 insertions(+), 53 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/16613/1
--
To view, visit http://gerrit.cloudera.org:8080/16613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3456396ac139c562ad9cd3ac1a624d8f35487518
Gerrit-Change-Number: 16613
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman 

[Impala-ASF-CR] IMPALA-10244: Make non-scalable failures to dequeue observable.

[Andrew Sherman (Code Review)]

Andrew Sherman has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/16613 )

Change subject: IMPALA-10244: Make non-scalable failures to dequeue observable.
..

IMPALA-10244: Make non-scalable failures to dequeue observable.

One of the important ways to observe Impala throughput is by looking at
when queries are queued. This can be an indication that more resources
should be added to the cluster by adding more executor groups. This is
only a good strategy if adding more resources will help with the current
workload. In some situations the head of the query queue cannot be
executed because of resource constraints on the coordinator. In these
cases the coordinator is the bottleneck so adding more executor groups
will not help. This change is to make these cases observable by adding a
new counter which is incremented when a dequeue fails because of
resource constraints that would not be resolved by adding more executor
groups.

The two cases that cause the counter to be incremented are:
- when there are not enough admission control slots on the coordinator
- when there is not enough memory on the coordinator
but it is possible that other conditions may be added in future.

TESTING:
  Added new unit tests.
  Ran all end-to-end tests.

Change-Id: I3456396ac139c562ad9cd3ac1a624d8f35487518
---
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M common/thrift/metrics.json
M tests/custom_cluster/test_admission_controller.py
5 files changed, 157 insertions(+), 53 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/16613/2
--
To view, visit http://gerrit.cloudera.org:8080/16613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3456396ac139c562ad9cd3ac1a624d8f35487518
Gerrit-Change-Number: 16613
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 

[Impala-ASF-CR] IMPALA-7714: try to avoid be test crash in statestore

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/16341 )

Change subject: IMPALA-7714: try to avoid be test crash in statestore
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/16341/1/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:

http://gerrit.cloudera.org:8080/#/c/16341/1/be/src/statestore/statestore.cc@396
PS1, Line 396:   // IMPALA-7714: log warning to aid debugging.
Should the logging go before the DCHECK?



--
To view, visit http://gerrit.cloudera.org:8080/16341
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id4363a93addb8a808d292906cac44ebd25c16889
Gerrit-Change-Number: 16341
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Fri, 14 Aug 2020 22:10:31 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7714: try to avoid be test crash in statestore

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/16341 )

Change subject: IMPALA-7714: try to avoid be test crash in statestore
..


Patch Set 2: Code-Review+2

LGTM


--
To view, visit http://gerrit.cloudera.org:8080/16341
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id4363a93addb8a808d292906cac44ebd25c16889
Gerrit-Change-Number: 16341
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 14 Aug 2020 23:31:01 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10244: Make non-scalable failures to dequeue observable.

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/16613 )

Change subject: IMPALA-10244: Make non-scalable failures to dequeue observable.
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/16613/2/be/src/scheduling/admission-controller-test.cc
File be/src/scheduling/admission-controller-test.cc:

http://gerrit.cloudera.org:8080/#/c/16613/2/be/src/scheduling/admission-controller-test.cc@382
PS2, Line 382: CanAdmitRequestMemory
> can you add a test where cannot_scale_with_executors is true if not enough
There is a test as part of 'Test 2' of DedicatedCoordAdmissionChecks on line 
944, do we need more than that?


http://gerrit.cloudera.org:8080/#/c/16613/2/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

http://gerrit.cloudera.org:8080/#/c/16613/2/be/src/scheduling/admission-controller.cc@1785
PS2, Line 1785: Increment
> Not sure if it is possible to implement this non-trivially but should we on
I understand what you mean these numbers can get quite big. But I am not sure 
how to do this without some complexity. Do you have a suggestion?


http://gerrit.cloudera.org:8080/#/c/16613/2/common/thrift/metrics.json
File common/thrift/metrics.json:

http://gerrit.cloudera.org:8080/#/c/16613/2/common/thrift/metrics.json@2731
PS2, Line 2731: Query dequeue failed because of a non-scalable limit
> nit: just thinking out aloud here: technically it is scalable, and we can p
I originally planned to have the name be related to "coordinator" but 
(elsewhere) Tim Armstrong suggested making it more generic (assuming I 
understood him correctly). It is true that today it is always a coordinator 
limit but I suppose it could be something else in future.



--
To view, visit http://gerrit.cloudera.org:8080/16613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3456396ac139c562ad9cd3ac1a624d8f35487518
Gerrit-Change-Number: 16613
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Thu, 22 Oct 2020 01:13:22 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9909: Print body of http error code in Impala Shell.

[Andrew Sherman (Code Review)]

Andrew Sherman has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/16269


Change subject: IMPALA-9909: Print body of http error code in Impala Shell.
..

IMPALA-9909: Print body of http error code in Impala Shell.

Make Impala Shell closer to Impyla by printing the body of any http
error code message received when using hs2-over-http. The common case is
that there is nothing in the body, in which case the behavior is
unchanged.

TESTING
 Added a test for the new functionality.
 Ran all end-to-end tests.

Change-Id: Iabc45eda0b87ca694b8359148cda6a7c1d5a8fff
---
M shell/ImpalaHttpClient.py
M tests/shell/test_shell_interactive.py
2 files changed, 88 insertions(+), 25 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/16269/3
--
To view, visit http://gerrit.cloudera.org:8080/16269
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iabc45eda0b87ca694b8359148cda6a7c1d5a8fff
Gerrit-Change-Number: 16269
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman 

[Impala-ASF-CR] IMPALA-9909: Print body of http error code in Impala Shell.

[Andrew Sherman (Code Review)]

Andrew Sherman has uploaded a new patch set (#4). ( 
http://gerrit.cloudera.org:8080/16269 )

Change subject: IMPALA-9909: Print body of http error code in Impala Shell.
..

IMPALA-9909: Print body of http error code in Impala Shell.

Make Impala Shell closer to Impyla by printing the body of any http
error code message received when using hs2-over-http. The common case is
that there is nothing in the body, in which case the behavior is
unchanged.

TESTING
 Added a test for the new functionality.
 Ran all end-to-end tests.

Change-Id: Iabc45eda0b87ca694b8359148cda6a7c1d5a8fff
---
M shell/ImpalaHttpClient.py
M tests/shell/test_shell_interactive.py
2 files changed, 90 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/16269/4
--
To view, visit http://gerrit.cloudera.org:8080/16269
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iabc45eda0b87ca694b8359148cda6a7c1d5a8fff
Gerrit-Change-Number: 16269
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 

[Impala-ASF-CR] IMPALA-10052: Expose daemon health endpoint for statestore and catalog

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/16295 )

Change subject: IMPALA-10052: Expose daemon health endpoint for statestore and 
catalog
..


Patch Set 2: Code-Review+2

LGTM


--
To view, visit http://gerrit.cloudera.org:8080/16295
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7714734df8e50dabbbebcb77a86a5a00bd13bf7c
Gerrit-Change-Number: 16295
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Comment-Date: Tue, 11 Aug 2020 02:01:02 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9540 Test that Impala Shell no longer sends duplicate "Host" headers in http mode.

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15752 )

Change subject: IMPALA-9540 Test that Impala Shell no longer sends duplicate 
"Host" headers in http mode.
..


Patch Set 5: Code-Review+2

Bring forward +2
I rebased and ran all tests


--
To view, visit http://gerrit.cloudera.org:8080/15752
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82996015d0205923e854dac8bb88604778684c46
Gerrit-Change-Number: 15752
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Tue, 30 Jun 2020 16:42:08 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9540 Test that Impala Shell no longer sends duplicate "Host" headers in http mode.

[Andrew Sherman (Code Review)]

Andrew Sherman has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/15752 )

Change subject: IMPALA-9540 Test that Impala Shell no longer sends duplicate 
"Host" headers in http mode.
..

IMPALA-9540 Test that Impala Shell no longer sends duplicate "Host" headers in 
http mode.

Many http servers will not accept an http request that has multiple
copies of the "Host" header. A recent toolchain change patches
Thrift so that will not send the extraneous header (in THttpClient).
This change tests that the duplicate headers are not sent,

TESTING:
  Ran all end-to-end tests.
  Rewrote an existing Shell test to check that only one "Host" header
  is sent.

Change-Id: I82996015d0205923e854dac8bb88604778684c46
---
M tests/shell/test_shell_interactive.py
M tests/shell/util.py
2 files changed, 54 insertions(+), 35 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/15752/3
--
To view, visit http://gerrit.cloudera.org:8080/15752
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I82996015d0205923e854dac8bb88604778684c46
Gerrit-Change-Number: 15752
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins 

[Impala-ASF-CR] IMPALA-9540 Test that Impala Shell no longer sends duplicate "Host" headers in http mode.

[Andrew Sherman (Code Review)]

Andrew Sherman has uploaded a new patch set (#4). ( 
http://gerrit.cloudera.org:8080/15752 )

Change subject: IMPALA-9540 Test that Impala Shell no longer sends duplicate 
"Host" headers in http mode.
..

IMPALA-9540 Test that Impala Shell no longer sends duplicate "Host" headers in 
http mode.

Many http servers will not accept an http request that has multiple
copies of the "Host" header. A recent toolchain change patches
Thrift so that will not send the extraneous header (in THttpClient).
This change tests that the duplicate headers are not sent,

TESTING:
  Ran all end-to-end tests.
  Rewrote an existing Shell test to check that only one "Host" header
  is sent.

Change-Id: I82996015d0205923e854dac8bb88604778684c46
---
M tests/shell/test_shell_interactive.py
M tests/shell/util.py
2 files changed, 55 insertions(+), 35 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/15752/4
--
To view, visit http://gerrit.cloudera.org:8080/15752
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I82996015d0205923e854dac8bb88604778684c46
Gerrit-Change-Number: 15752
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins 

[Impala-ASF-CR] IMPALA-9540 Test that Impala Shell no longer sends duplicate "Host" headers in http mode.

[Andrew Sherman (Code Review)]

Andrew Sherman has uploaded a new patch set (#5). ( 
http://gerrit.cloudera.org:8080/15752 )

Change subject: IMPALA-9540 Test that Impala Shell no longer sends duplicate 
"Host" headers in http mode.
..

IMPALA-9540 Test that Impala Shell no longer sends duplicate "Host" headers in 
http mode.

Many http servers will not accept an http request that has multiple
copies of the "Host" header. A recent toolchain change patches
Thrift so that will not send the extraneous header (in THttpClient).
This change tests that the duplicate headers are not sent,

TESTING:
  Ran all end-to-end tests.
  Rewrote an existing Shell test to check that only one "Host" header
  is sent.

Change-Id: I82996015d0205923e854dac8bb88604778684c46
---
M tests/shell/test_shell_interactive.py
M tests/shell/util.py
2 files changed, 54 insertions(+), 35 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/15752/5
--
To view, visit http://gerrit.cloudera.org:8080/15752
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I82996015d0205923e854dac8bb88604778684c46
Gerrit-Change-Number: 15752
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins 

[Impala-ASF-CR] IMPALA-3695: Remove KUDU IS SUPPORTED

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/16085 )

Change subject: IMPALA-3695: Remove KUDU_IS_SUPPORTED
..


Patch Set 4: Code-Review+2

This looks safe and a good cleanup.
I did look for things that were missed and only found
common/thrift/generate_error_codes.py:  ("KUDU_NOT_SUPPORTED_ON_OS", 74, "Kudu 
is not supported on this operating system
I see that TErrorCode::KUDU_NOT_SUPPORTED_ON_OS can still be returned in 
CheckKuduAvailability()
Is it worth looking at removing this too? If we wanted to do that it could 
certainly be left for a future change.


--
To view, visit http://gerrit.cloudera.org:8080/16085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I059d7a42798c38b570f25283663c284f2fcee517
Gerrit-Change-Number: 16085
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Wed, 17 Jun 2020 17:29:56 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9871: Simplify OS version matching for SLES in bootstrap toolchain.py

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/16102 )

Change subject: IMPALA-9871: Simplify OS version matching for SLES in 
bootstrap_toolchain.py
..


Patch Set 1: Code-Review+2

LGTM


--
To view, visit http://gerrit.cloudera.org:8080/16102
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id9ada210b915050fbceebb7364e130116e9244d0
Gerrit-Change-Number: 16102
Gerrit-PatchSet: 1
Gerrit-Owner: Laszlo Gaal 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Comment-Date: Tue, 23 Jun 2020 15:39:14 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9829: Add Write Metrics for Spilling

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/16083 )

Change subject: IMPALA-9829: Add Write Metrics for Spilling
..


Patch Set 10:

(13 comments)

Good change! I like the detailed unit tests.
I think a few cosmetic changes are all that is needed.
This may seem like a picky nit but in Impala code, comments start with a 
capital letter and end with a period.
This may seem weirdly prescriptive but it does enhance readability.

http://gerrit.cloudera.org:8080/#/c/16083/10//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/16083/10//COMMIT_MSG@9
PS10, Line 9: Three types of metrics are added in disk-io-mgr:
Nit: in the commit message it can be best to be more high level, and describe 
what is added in more descriptive terms.


http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc
File be/src/runtime/io/disk-io-mgr-test.cc:

http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc@1648
PS10, Line 1648: // the write operations
Finish comment with a period.


http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc@1664
PS10, Line 1664:   // Reset the Metric if it exists
Add period.


http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc@1686
PS10, Line 1686:   // Issue a number of writes to the disks
Add period.


http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc@1706
PS10, Line 1706:   // Check the count and max/min of the histogram metric
Add period.


http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc@1712
PS10, Line 1712: // The count should be added by num_ranges/num_disks per 
disk
Add period.


http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc@1714
PS10, Line 1714: // Check if the min and max of write size are the same as 
the written len
Add period.


http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc@1732
PS10, Line 1732: // Issue a writing operation to a non-existent tmp file path
Add periods


http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc@1737
PS10, Line 1737:   string tmp_file = 
"/tmp/disk_io_mgr_test/MetricsOfWriteIoError";
Another test uses "/non-existent/file.txt" to indicate a non-existing file, 
this makes it clearer what is happening


http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc@1739
PS10, Line 1739:   // Reset the Metric if it exists
Add period.


http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc@1746
PS10, Line 1746:   // Remove the path in case it exists
Add period.


http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc@1767
PS10, Line 1767:   // One IO Error should be added to the metrics counter
Add period.


http://gerrit.cloudera.org:8080/#/c/16083/10/common/thrift/metrics.json
File common/thrift/metrics.json:

http://gerrit.cloudera.org:8080/#/c/16083/10/common/thrift/metrics.json@663
PS10, Line 663: "description": "The number of write io error on disk.",
Should be "errors".



--
To view, visit http://gerrit.cloudera.org:8080/16083
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I152b9c5339cedabe33f8873a2bbf651aa5dbb914
Gerrit-Change-Number: 16083
Gerrit-PatchSet: 10
Gerrit-Owner: Yida Wu 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Yida Wu 
Gerrit-Comment-Date: Thu, 25 Jun 2020 17:36:10 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9790: option to use resolved hostname everywhere

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/16108 )

Change subject: IMPALA-9790: option to use resolved hostname everywhere
..


Patch Set 2: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/16108
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d5cb9c68c60ce8dc838cde9dcf1c590017f5c9a
Gerrit-Change-Number: 16108
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Comment-Date: Fri, 26 Jun 2020 18:54:18 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9829: Add Write Metrics for Spilling

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/16083 )

Change subject: IMPALA-9829: Add Write Metrics for Spilling
..


Patch Set 13: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/16083
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I152b9c5339cedabe33f8873a2bbf651aa5dbb914
Gerrit-Change-Number: 16083
Gerrit-PatchSet: 13
Gerrit-Owner: Yida Wu 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Yida Wu 
Gerrit-Comment-Date: Fri, 26 Jun 2020 17:46:16 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9885: debug action for planner

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/16104 )

Change subject: IMPALA-9885: debug action for planner
..


Patch Set 2: Code-Review+2

LGTM


--
To view, visit http://gerrit.cloudera.org:8080/16104
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e97c9a427db600fb7334365261c649b33c90b64
Gerrit-Change-Number: 16104
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Tue, 23 Jun 2020 20:24:44 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9077: Remove scalable admission control configs

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/16039 )

Change subject: IMPALA-9077: Remove scalable admission control configs
..


Patch Set 4: Code-Review+2

(2 comments)

This looks pretty straightforward.

http://gerrit.cloudera.org:8080/#/c/16039/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/16039/4//COMMIT_MSG@14
PS4, Line 14: This patch reverts the functionality related to those configs but
Nit: I would say it "removes" the functionality rather than "reverts".
A revert is usually what we say for a temporary removal?
I suppose you could also explain that this removal is to make it easier to 
enhance Admission Control using Executor Groups which has turned out to be a 
useful building block.


http://gerrit.cloudera.org:8080/#/c/16039/4/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

http://gerrit.cloudera.org:8080/#/c/16039/4/be/src/scheduling/admission-controller.cc@1824
PS4, Line 1824: bool AdmissionController::PoolLimitsRunningQueriesCount(const 
TPoolConfig& pool_config) {
I see you are keeping some utility functions like this. Probably they do 
increase the readability of the code so I agree with this.



--
To view, visit http://gerrit.cloudera.org:8080/16039
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9bd63f03758a6c4eebb99c64ee67e60cb56b5ac
Gerrit-Change-Number: 16039
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 08 Jun 2020 23:28:31 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10006: handle non-writable /opt/impala/logs

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/16237 )

Change subject: IMPALA-10006: handle non-writable /opt/impala/logs
..


Patch Set 2: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/16237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If32d6eef75422b51f8877478bbfb1a709c02f756
Gerrit-Change-Number: 16237
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Thu, 30 Jul 2020 17:17:04 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10373: Run impala docker containers with uid/gid 1000

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/16807 )

Change subject: IMPALA-10373: Run impala docker containers with uid/gid 1000
..


Patch Set 1: Code-Review+2

LGTM


--
To view, visit http://gerrit.cloudera.org:8080/16807
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b846ca5fb2c55ac1707b9581cee18447467b41
Gerrit-Change-Number: 16807
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Wed, 02 Dec 2020 17:25:46 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10309: Use sleep time from a Retry-After header in Impala Shell

[Andrew Sherman (Code Review)]

Andrew Sherman has uploaded a new patch set (#4). ( 
http://gerrit.cloudera.org:8080/16702 )

Change subject: IMPALA-10309: Use sleep time from a Retry-After header in 
Impala Shell
..

IMPALA-10309: Use sleep time from a Retry-After header in Impala Shell

When Impala Shell receives an http error message (that is a message with
http code greater than or equal to 300), it may sleep for a time before
retrying. If the message contains a 'Retry-After' header that has an
integer value, then this will be used as the time for which to sleep.

The implementation is to use a new HttpError exception (similar to that
used in Impyla) which includes more information from the error message
(including the headers) so that catchers of the exception can use the
'Retry-After' header if appropriate.

TESTING:
Hand testing with a proxy that uses the 'Retry-After' header.
Added new tests that use the fault injection framework in
test_hs2_fault_injection.py

Change-Id: I2b4226e7723d585d61deb4d1d6777aac901bfd93
---
M shell/ImpalaHttpClient.py
M shell/impala_client.py
M shell/shell_exceptions.py
M tests/custom_cluster/test_hs2_fault_injection.py
4 files changed, 143 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/16702/4
--
To view, visit http://gerrit.cloudera.org:8080/16702
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2b4226e7723d585d61deb4d1d6777aac901bfd93
Gerrit-Change-Number: 16702
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Impala Public Jenkins 

[Impala-ASF-CR] IMPALA-10309: Use sleep time from a Retry-After header in Impala Shell

[Andrew Sherman (Code Review)]

Andrew Sherman has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/16702 )

Change subject: IMPALA-10309: Use sleep time from a Retry-After header in 
Impala Shell
..

IMPALA-10309: Use sleep time from a Retry-After header in Impala Shell

When Impala Shell receives an http error message (that is a message with
http code greater than or equal to 300), it may sleep for a time before
retrying. If the message contains a 'Retry-After' header that has an
integer value, then this will be used as the time for which to sleep.

The implementation is to use a new HttpError exception (similar to that
used in Impyla) which includes more information from the error message
(including the headers) so that catchers of the exception can use the
'Retry-After' header if appropriate.

TESTING:
Hand testing with a proxy that uses the 'Retry-After' header.
Added new tests that use the fault injection framework in
test_hs2_fault_injection.py

Change-Id: I2b4226e7723d585d61deb4d1d6777aac901bfd93
---
M shell/ImpalaHttpClient.py
M shell/impala_client.py
M shell/shell_exceptions.py
M tests/custom_cluster/test_hs2_fault_injection.py
4 files changed, 142 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/16702/3
--
To view, visit http://gerrit.cloudera.org:8080/16702
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2b4226e7723d585d61deb4d1d6777aac901bfd93
Gerrit-Change-Number: 16702
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Impala Public Jenkins 

[Impala-ASF-CR] IMPALA-10309: Use sleep time from a Retry-After header in Impala Shell

[Andrew Sherman (Code Review)]

Andrew Sherman has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/16702


Change subject: IMPALA-10309: Use sleep time from a Retry-After header in 
Impala Shell
..

IMPALA-10309: Use sleep time from a Retry-After header in Impala Shell

When Impala Shell receives an http error message (that is a message with
http code greater than or equal to 300), it may sleep for a time before
retrying. If the message contains a 'Retry-After' header that has an
integer value, then this will be used as the time for which to sleep.

The implementation is to use a new HttpError exception (similar to that
used in Impyla) which includes more information from the error message
(including the headers) so that catchers of the exception can use the
'Retry-After' header if appropriate.

TESTING:
Hand testing with a proxy that uses the 'Retry-After' header.
Added new tests that use the fault injection framework in
test_hs2_fault_injection.py

Change-Id: I2b4226e7723d585d61deb4d1d6777aac901bfd93
---
M shell/ImpalaHttpClient.py
M shell/impala_client.py
M shell/shell_exceptions.py
M tests/custom_cluster/test_hs2_fault_injection.py
4 files changed, 140 insertions(+), 12 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/16702/2
--
To view, visit http://gerrit.cloudera.org:8080/16702
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2b4226e7723d585d61deb4d1d6777aac901bfd93
Gerrit-Change-Number: 16702
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman 

[Impala-ASF-CR] IMPALA-10249: Fix the flaky TestImpalaShell.test queries closed test.

[Andrew Sherman (Code Review)]

Andrew Sherman has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/16743


Change subject: IMPALA-10249: Fix the flaky TestImpalaShell.test_queries_closed 
test.
..

IMPALA-10249: Fix the flaky TestImpalaShell.test_queries_closed test.

This test for IMPALA-897 is testing that queries run by Impala Shell
from a script file are closed correctly.  This is tested by an assertion
that there is one in-flight query during execution of a script
containing several queries. The test then closes the shell and checks
that there are no in-flight queries. This is the assertion which failed.
Change this assertion to instead wait for the number of in-flight
queries to be zero. This avoids whatever race was causing the flakiness.

Change-Id: Ib0485097c34282523ed0df6faa143fee6f74676d
---
M tests/shell/test_shell_commandline.py
1 file changed, 1 insertion(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/16743/1
--
To view, visit http://gerrit.cloudera.org:8080/16743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib0485097c34282523ed0df6faa143fee6f74676d
Gerrit-Change-Number: 16743
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman 

[Impala-ASF-CR] IMPALA-10145,IMPALA-10299: Bump impala-shell thrift version to 0.11.0-p4

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/16700 )

Change subject: IMPALA-10145,IMPALA-10299: Bump impala-shell thrift version to 
0.11.0-p4
..


Patch Set 2: Code-Review+2

LGTM


--
To view, visit http://gerrit.cloudera.org:8080/16700
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0f9898639b5648658efc2d3c5c0ee4721fb85776
Gerrit-Change-Number: 16700
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Mon, 09 Nov 2020 17:56:54 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10244: Make non-scalable failures to dequeue observable.

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/16613 )

Change subject: IMPALA-10244: Make non-scalable failures to dequeue observable.
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/16613/2/common/thrift/metrics.json
File common/thrift/metrics.json:

http://gerrit.cloudera.org:8080/#/c/16613/2/common/thrift/metrics.json@2731
PS2, Line 2731: Query dequeue failed because of a non-scalable limit
> I originally planned to have the name be related to "coordinator" but (else
I will change to use the name, suggested by Quigan


http://gerrit.cloudera.org:8080/#/c/16613/2/common/thrift/metrics.json@2734
PS2, Line 2734: admission-controller.total-dequeue-failed-cannot-scale"
> I had the same impression here as well. The new counter added counts the #
Thanks! I will use this name



--
To view, visit http://gerrit.cloudera.org:8080/16613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3456396ac139c562ad9cd3ac1a624d8f35487518
Gerrit-Change-Number: 16613
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Qifan Chen 
Gerrit-Comment-Date: Thu, 22 Oct 2020 21:15:13 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10244: Make non-scalable failures to dequeue observable.

[Andrew Sherman (Code Review)]

Andrew Sherman has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/16613 )

Change subject: IMPALA-10244: Make non-scalable failures to dequeue observable.
..

IMPALA-10244: Make non-scalable failures to dequeue observable.

One of the important ways to observe Impala throughput is by looking at
when queries are queued. This can be an indication that more resources
should be added to the cluster by adding more executor groups. This is
only a good strategy if adding more resources will help with the current
workload. In some situations the head of the query queue cannot be
executed because of resource constraints on the coordinator. In these
cases the coordinator is the bottleneck so adding more executor groups
will not help. This change is to make these cases observable by adding a
new counter which is incremented when a dequeue fails because of
resource constraints that would not be resolved by adding more executor
groups.

The two cases that cause the counter to be incremented are:
- when there are not enough admission control slots on the coordinator
- when there is not enough memory on the coordinator
but it is possible that other conditions may be added in future.

TESTING:
  Added new unit tests.
  Ran all end-to-end tests.

Change-Id: I3456396ac139c562ad9cd3ac1a624d8f35487518
---
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M common/thrift/metrics.json
M tests/custom_cluster/test_admission_controller.py
5 files changed, 159 insertions(+), 55 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/16613/3
--
To view, visit http://gerrit.cloudera.org:8080/16613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3456396ac139c562ad9cd3ac1a624d8f35487518
Gerrit-Change-Number: 16613
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 

[Impala-ASF-CR] IMPALA-10244: Make non-scalable failures to dequeue observable.

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/16613 )

Change subject: IMPALA-10244: Make non-scalable failures to dequeue observable.
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/16613/2/common/thrift/metrics.json
File common/thrift/metrics.json:

http://gerrit.cloudera.org:8080/#/c/16613/2/common/thrift/metrics.json@2734
PS2, Line 2734: admission-controller.total-dequeue-failed-cannot-scale"
> Thanks! I will use this name
Actually admission-controller.total-dequeue-failed-coordinator-limited
seems clearer



--
To view, visit http://gerrit.cloudera.org:8080/16613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3456396ac139c562ad9cd3ac1a624d8f35487518
Gerrit-Change-Number: 16613
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Qifan Chen 
Gerrit-Comment-Date: Thu, 22 Oct 2020 21:28:44 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10244: Make non-scalable failures to dequeue observable.

[Andrew Sherman (Code Review)]

Andrew Sherman has uploaded a new patch set (#4). ( 
http://gerrit.cloudera.org:8080/16613 )

Change subject: IMPALA-10244: Make non-scalable failures to dequeue observable.
..

IMPALA-10244: Make non-scalable failures to dequeue observable.

One of the important ways to observe Impala throughput is by looking at
when queries are queued. This can be an indication that more resources
should be added to the cluster by adding more executor groups. This is
only a good strategy if adding more resources will help with the current
workload. In some situations the head of the query queue cannot be
executed because of resource constraints on the coordinator. In these
cases the coordinator is the bottleneck so adding more executor groups
will not help. This change is to make these cases observable by adding a
new counter which is incremented when a dequeue fails because of
resource constraints on the coordinator.

The two cases that cause the counter to be incremented are:
- when there are not enough admission control slots on the coordinator
- when there is not enough memory on the coordinator
but it is possible that other conditions may be added in future.

TESTING:
Added new unit tests.
Ran all end-to-end tests.

Change-Id: I3456396ac139c562ad9cd3ac1a624d8f35487518
---
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M common/thrift/metrics.json
M tests/custom_cluster/test_admission_controller.py
5 files changed, 161 insertions(+), 55 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/16613/4
--
To view, visit http://gerrit.cloudera.org:8080/16613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3456396ac139c562ad9cd3ac1a624d8f35487518
Gerrit-Change-Number: 16613
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Qifan Chen 

[Impala-ASF-CR] IMPALA-10244: Make non-scalable failures to dequeue observable.

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/16613 )

Change subject: IMPALA-10244: Make non-scalable failures to dequeue observable.
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/16613/2/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

http://gerrit.cloudera.org:8080/#/c/16613/2/be/src/scheduling/admission-controller.cc@1785
PS2, Line 1785: Increment
> we can add a bool to the QueueNode to keep track of this
That's a clever idea, so I could make sure that for every queued query, my new 
metric would be bumped only once. But from my point of view this makes it less 
observable. If a query gets stuck at the front of the queue for some time I 
actually want to be able to observe that. So I could add a timestamp to 
QueueNode and only bump the counter every X seconds. But I'm not sure this 
complexity is worth it. In some ways the raw number does reflect something that 
is actually happening. What do you think?



--
To view, visit http://gerrit.cloudera.org:8080/16613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3456396ac139c562ad9cd3ac1a624d8f35487518
Gerrit-Change-Number: 16613
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Qifan Chen 
Gerrit-Comment-Date: Fri, 23 Oct 2020 00:33:07 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10303: Fix warnings from impala-shell with --quiet

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/16673 )

Change subject: IMPALA-10303: Fix warnings from impala-shell with --quiet
..


Patch Set 2: Code-Review+2

LGTM


--
To view, visit http://gerrit.cloudera.org:8080/16673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e94c9445ffba159725bacd6f6bc36f7c91b88fe
Gerrit-Change-Number: 16673
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Thu, 29 Oct 2020 20:57:23 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10447: Add a newline when exporting shell output to a file.

[Andrew Sherman (Code Review)]

Andrew Sherman has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/16966


Change subject: IMPALA-10447: Add a newline when exporting shell output to a 
file.
..

IMPALA-10447: Add a newline when exporting shell output to a file.

Impala shell outputs a batch of rows using OutputStream. Inside
OutputStream, output to a file is handled slightly differently from
output that is written to stdout. When writing to stdout we use print()
(which appends a newline) while when writing to a file we use write()
(which adds nothing). This difference was introduced in IMPALA-3343 so
this bug may be a regression introduced then. To ensure that output is
the same in either case we need to add a newline after writing each
batch of rows to a file.

TESTING:
Added a new test for this case.

Change-Id: I078a06c54e0834bc1f898626afbfff4ded579fa9
---
M shell/shell_output.py
M tests/shell/test_shell_commandline.py
2 files changed, 30 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/16966/1
--
To view, visit http://gerrit.cloudera.org:8080/16966
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I078a06c54e0834bc1f898626afbfff4ded579fa9
Gerrit-Change-Number: 16966
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman