[Impala-ASF-CR] IMPALA-4970: Record identity of largest latency ExecQueryFInstances()

2018-05-25 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10490 )

Change subject: IMPALA-4970: Record identity of largest latency 
ExecQueryFInstances()
..


Patch Set 6: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf9ce0d087c91c2e92ba5be08c96cc5364ae38dc
Gerrit-Change-Number: 10490
Gerrit-PatchSet: 6
Gerrit-Owner: Rahul Shivu Mahadev 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Rahul Shivu Mahadev 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 25 May 2018 22:13:07 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4970: Record identity of largest latency ExecQueryFInstances()

2018-05-25 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/10490 )

Change subject: IMPALA-4970: Record identity of largest latency 
ExecQueryFInstances()
..

IMPALA-4970: Record identity of largest latency ExecQueryFInstances()

Tracked the slowest ExecQueryFInstances() RPC to executors and
added the identity of that executor to the RuntimeProfile.

Testing: Manually tested to verify working.
Change-Id: Icf9ce0d087c91c2e92ba5be08c96cc5364ae38dc
Reviewed-on: http://gerrit.cloudera.org:8080/10490
Reviewed-by: Sailesh Mukil 
Tested-by: Impala Public Jenkins 
---
M be/src/runtime/coordinator.cc
1 file changed, 9 insertions(+), 0 deletions(-)

Approvals:
  Sailesh Mukil: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Icf9ce0d087c91c2e92ba5be08c96cc5364ae38dc
Gerrit-Change-Number: 10490
Gerrit-PatchSet: 7
Gerrit-Owner: Rahul Shivu Mahadev 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Rahul Shivu Mahadev 
Gerrit-Reviewer: Sailesh Mukil

[Impala-ASF-CR] IMPALA-4970: Record identity of largest latency ExecQueryFInstances()

2018-05-25 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10490 )

Change subject: IMPALA-4970: Record identity of largest latency 
ExecQueryFInstances()
..


Patch Set 6:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2558/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf9ce0d087c91c2e92ba5be08c96cc5364ae38dc
Gerrit-Change-Number: 10490
Gerrit-PatchSet: 6
Gerrit-Owner: Rahul Shivu Mahadev 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Rahul Shivu Mahadev 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 25 May 2018 18:49:04 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4970: Record identity of largest latency ExecQueryFInstances()

2018-05-25 Thread Sailesh Mukil (Code Review) 
Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10490 )

Change subject: IMPALA-4970: Record identity of largest latency 
ExecQueryFInstances()
..


Patch Set 6: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf9ce0d087c91c2e92ba5be08c96cc5364ae38dc
Gerrit-Change-Number: 10490
Gerrit-PatchSet: 6
Gerrit-Owner: Rahul Shivu Mahadev 
Gerrit-Reviewer: Rahul Shivu Mahadev 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 25 May 2018 18:47:20 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4970: Record identity of largest latency ExecQueryFInstances()

2018-05-25 Thread Sailesh Mukil (Code Review) 
Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10490 )

Change subject: IMPALA-4970: Record identity of largest latency 
ExecQueryFInstances()
..


Patch Set 5: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf9ce0d087c91c2e92ba5be08c96cc5364ae38dc
Gerrit-Change-Number: 10490
Gerrit-PatchSet: 5
Gerrit-Owner: Rahul Shivu Mahadev 
Gerrit-Reviewer: Rahul Shivu Mahadev 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 25 May 2018 18:46:59 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4970: Record identity of largest latency ExecQueryFInstances()

2018-05-25 Thread Rahul Shivu Mahadev (Code Review) 
Rahul Shivu Mahadev has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10490 )

Change subject: IMPALA-4970: Record identity of largest latency 
ExecQueryFInstances()
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10490/3/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

http://gerrit.cloudera.org:8080/#/c/10490/3/be/src/runtime/coordinator.cc@365
PS3, Line 365: if (backend_state->rpc_latency() > max_latency) {
> Just one last comment:
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf9ce0d087c91c2e92ba5be08c96cc5364ae38dc
Gerrit-Change-Number: 10490
Gerrit-PatchSet: 5
Gerrit-Owner: Rahul Shivu Mahadev 
Gerrit-Reviewer: Rahul Shivu Mahadev 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 25 May 2018 17:12:17 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4970: Record identity of largest latency ExecQueryFInstances()

2018-05-25 Thread Rahul Shivu Mahadev (Code Review) 
Hello Sailesh Mukil,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10490

to look at the new patch set (#4).

Change subject: IMPALA-4970: Record identity of largest latency 
ExecQueryFInstances()
..

IMPALA-4970: Record identity of largest latency ExecQueryFInstances()

Tracked the slowest ExecQueryFInstances() RPC to executors and
added the identity of that executor to the RuntimeProfile.

Testing: Manually tested to verify working.
Change-Id: Icf9ce0d087c91c2e92ba5be08c96cc5364ae38dc
---
M be/src/runtime/coordinator.cc
1 file changed, 9 insertions(+), 0 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf9ce0d087c91c2e92ba5be08c96cc5364ae38dc
Gerrit-Change-Number: 10490
Gerrit-PatchSet: 4
Gerrit-Owner: Rahul Shivu Mahadev 
Gerrit-Reviewer: Rahul Shivu Mahadev 
Gerrit-Reviewer: Sailesh Mukil

[Impala-ASF-CR] IMPALA-4970: Record identity of largest latency ExecQueryFInstances()

2018-05-25 Thread Sailesh Mukil (Code Review) 
Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10490 )

Change subject: IMPALA-4970: Record identity of largest latency 
ExecQueryFInstances()
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10490/2/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

http://gerrit.cloudera.org:8080/#/c/10490/2/be/src/runtime/coordinator.cc@357
PS2, Line 357: int max_latency = 0;
> I won't be able to do that as we need to iterate and find the max latency
You're right. My bad.


http://gerrit.cloudera.org:8080/#/c/10490/3/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

http://gerrit.cloudera.org:8080/#/c/10490/3/be/src/runtime/coordinator.cc@365
PS3, Line 365: if (backend_state->rpc_latency() > max_latency) {
Just one last comment:
Could you add a comment above this line that says something like:
"Find the backend that takes the most time to acknowledge the 
ExecQueryFInstances() RPC"



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf9ce0d087c91c2e92ba5be08c96cc5364ae38dc
Gerrit-Change-Number: 10490
Gerrit-PatchSet: 3
Gerrit-Owner: Rahul Shivu Mahadev 
Gerrit-Reviewer: Rahul Shivu Mahadev 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 25 May 2018 16:44:58 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4970: Record identity of largest latency ExecQueryFInstances()

2018-05-25 Thread Rahul Shivu Mahadev (Code Review) 
Rahul Shivu Mahadev has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10490 )

Change subject: IMPALA-4970: Record identity of largest latency 
ExecQueryFInstances()
..


Patch Set 3:

Run changes with test-suite and all tests passed


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf9ce0d087c91c2e92ba5be08c96cc5364ae38dc
Gerrit-Change-Number: 10490
Gerrit-PatchSet: 3
Gerrit-Owner: Rahul Shivu Mahadev 
Gerrit-Reviewer: Rahul Shivu Mahadev 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 25 May 2018 16:37:12 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4970: Record identity of largest latency ExecQueryFInstances()

2018-05-24 Thread Rahul Shivu Mahadev (Code Review) 
Rahul Shivu Mahadev has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10490 )

Change subject: IMPALA-4970: Record identity of largest latency 
ExecQueryFInstances()
..


Patch Set 3:

(9 comments)

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

http://gerrit.cloudera.org:8080/#/c/10490/2//COMMIT_MSG@7
PS2, Line 7: IMPALA-4970: Record identity of largest latency 
ExecQueryFInstances()
   :
> nit: Commit message titles have to be in one line, so if it exceeds the cha
Done


http://gerrit.cloudera.org:8080/#/c/10490/2//COMMIT_MSG@10
PS2, Line 10: added the identity of that executor to the RuntimeProfile.
:
> nit: Tracked the slowest ExecQueryFInstances() RPC to executors and added t
Done


http://gerrit.cloudera.org:8080/#/c/10490/2//COMMIT_MSG@12
PS2, Line 12: Testing: Manually tested to verify working.
> Can you add a note saying how you manually tested it?
Done


http://gerrit.cloudera.org:8080/#/c/10490/2/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

http://gerrit.cloudera.org:8080/#/c/10490/2/be/src/runtime/coordinator.cc@357
PS2, Line 357: int max_latency = 0;
> Move this into the for loop just above L365 to limit the scope of its lifet
I won't be able to do that as we need to iterate and find the max latency


http://gerrit.cloudera.org:8080/#/c/10490/2/be/src/runtime/coordinator.cc@357
PS2, Line 357: 0;
> This can be 0, since we never expect a negative latency.
Done


http://gerrit.cloudera.org:8080/#/c/10490/2/be/src/runtime/coordinator.cc@367
PS2, Line 367: TNetworkAddressToString(backend_state->impalad_address());
> use the TNetworkAddressToString() utility here to print the impalad_address
Done


http://gerrit.cloudera.org:8080/#/c/10490/2/be/src/runtime/coordinator.cc@367
PS2, Line 367: max_latency_host = 
TNetworkAddressToString(backend_state->impalad_address());
> nit: long line. Keep lines to 90 chars, and make it a multi-line statement
Done


http://gerrit.cloudera.org:8080/#/c/10490/2/be/src/runtime/coordinator.cc@374
PS2, Line 374: n UpdateExec
> The previous line says "Backend startup latencies". So lets keep the terms
Done


http://gerrit.cloudera.org:8080/#/c/10490/2/be/src/runtime/coordinator.cc@373
PS2, Line 373: query_profile_->AddInfoString("Slowest Backend to startup", 
max_latency_host);
 :   return UpdateExecState(status, nullptr
> This can fit on one line.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf9ce0d087c91c2e92ba5be08c96cc5364ae38dc
Gerrit-Change-Number: 10490
Gerrit-PatchSet: 3
Gerrit-Owner: Rahul Shivu Mahadev 
Gerrit-Reviewer: Rahul Shivu Mahadev 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 24 May 2018 21:59:28 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4970: Record identity of largest latency ExecQueryFInstances()

2018-05-24 Thread Rahul Shivu Mahadev (Code Review) 
Hello Sailesh Mukil,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10490

to look at the new patch set (#3).

Change subject: IMPALA-4970: Record identity of largest latency 
ExecQueryFInstances()
..

IMPALA-4970: Record identity of largest latency ExecQueryFInstances()

Tracked the slowest ExecQueryFInstances() RPC to executors and
added the identity of that executor to the RuntimeProfile.

Testing: Manually tested to verify working.
Change-Id: Icf9ce0d087c91c2e92ba5be08c96cc5364ae38dc
---
M be/src/runtime/coordinator.cc
1 file changed, 7 insertions(+), 0 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf9ce0d087c91c2e92ba5be08c96cc5364ae38dc
Gerrit-Change-Number: 10490
Gerrit-PatchSet: 3
Gerrit-Owner: Rahul Shivu Mahadev 
Gerrit-Reviewer: Sailesh Mukil

[Impala-ASF-CR] IMPALA-4970: Record identity of largest latency ExecQueryFInstances() RPC per query.

2018-05-24 Thread Sailesh Mukil (Code Review) 
Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10490 )

Change subject: IMPALA-4970: Record identity of largest latency 
ExecQueryFInstances() RPC per query.
..


Patch Set 2:

(9 comments)

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

http://gerrit.cloudera.org:8080/#/c/10490/2//COMMIT_MSG@7
PS2, Line 7: IMPALA-4970: Record identity of largest latency 
ExecQueryFInstances() RPC per
   : query.
nit: Commit message titles have to be in one line, so if it exceeds the char 
limit, it's okay, but only the titles.


http://gerrit.cloudera.org:8080/#/c/10490/2//COMMIT_MSG@10
PS2, Line 10: Added code to track the slowest RPC to BackendState and 
identifying that node
: as the slowest node.
nit: Tracked the slowest ExecQueryFInstances() RPC to executors and added the 
identity of that executor to the RuntimeProfile.


http://gerrit.cloudera.org:8080/#/c/10490/2//COMMIT_MSG@12
PS2, Line 12:
Can you add a note saying how you manually tested it?

Add "Testing: ..."


http://gerrit.cloudera.org:8080/#/c/10490/2/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

http://gerrit.cloudera.org:8080/#/c/10490/2/be/src/runtime/coordinator.cc@357
PS2, Line 357: INT_MIN
This can be 0, since we never expect a negative latency.


http://gerrit.cloudera.org:8080/#/c/10490/2/be/src/runtime/coordinator.cc@357
PS2, Line 357: int max_latency = INT_MIN;
Move this into the for loop just above L365 to limit the scope of its lifetime.


http://gerrit.cloudera.org:8080/#/c/10490/2/be/src/runtime/coordinator.cc@367
PS2, Line 367: max_latency_host = backend_state->impalad_address().hostname + 
":" + std::to_string(backend_state->impalad_address().port);
nit: long line. Keep lines to 90 chars, and make it a multi-line statement if 
it doesn't fit.


http://gerrit.cloudera.org:8080/#/c/10490/2/be/src/runtime/coordinator.cc@367
PS2, Line 367: backend_state->impalad_address().hostname + ":" + 
std::to_string(backend_state->impalad_address().port);
use the TNetworkAddressToString() utility here to print the impalad_address()
Eg: Look at L686


http://gerrit.cloudera.org:8080/#/c/10490/2/be/src/runtime/coordinator.cc@373
PS2, Line 373: query_profile_->AddInfoString(
 :   "Slowest Host", max_latency_host);
This can fit on one line.


http://gerrit.cloudera.org:8080/#/c/10490/2/be/src/runtime/coordinator.cc@374
PS2, Line 374: Slowest Host
The previous line says "Backend startup latencies". So lets keep the terms 
consistent.
Maybe change this to "Slowest backend to startup" or something similar.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf9ce0d087c91c2e92ba5be08c96cc5364ae38dc
Gerrit-Change-Number: 10490
Gerrit-PatchSet: 2
Gerrit-Owner: Rahul Shivu Mahadev 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 24 May 2018 21:03:14 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4970: Record identity of largest latency ExecQueryFInstances() RPC per query.

2018-05-24 Thread Rahul Shivu Mahadev (Code Review) 
Hello Sailesh Mukil,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10490

to look at the new patch set (#2).

Change subject: IMPALA-4970: Record identity of largest latency 
ExecQueryFInstances() RPC per query.
..

IMPALA-4970: Record identity of largest latency ExecQueryFInstances() RPC per
query.

Added code to track the slowest RPC to BackendState and identifying that node
as the slowest node.

Change-Id: Icf9ce0d087c91c2e92ba5be08c96cc5364ae38dc
---
M be/src/runtime/coordinator.cc
1 file changed, 8 insertions(+), 0 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf9ce0d087c91c2e92ba5be08c96cc5364ae38dc
Gerrit-Change-Number: 10490
Gerrit-PatchSet: 2
Gerrit-Owner: Rahul Shivu Mahadev 
Gerrit-Reviewer: Sailesh Mukil

[Impala-ASF-CR] IMPALA-4970: Record identity of largest latency ExecQueryFInstances() RPC per query.

2018-05-24 Thread Rahul Shivu Mahadev (Code Review) 
Rahul Shivu Mahadev has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10490


Change subject: IMPALA-4970: Record identity of largest latency 
ExecQueryFInstances() RPC per query.
..

IMPALA-4970: Record identity of largest latency ExecQueryFInstances() RPC per
query.

Added code to track the slowest RPC to BackendState and identifying that node
as the slowest node.

Change-Id: Icf9ce0d087c91c2e92ba5be08c96cc5364ae38dc
---
M be/src/runtime/coordinator.cc
1 file changed, 8 insertions(+), 0 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icf9ce0d087c91c2e92ba5be08c96cc5364ae38dc
Gerrit-Change-Number: 10490
Gerrit-PatchSet: 1
Gerrit-Owner: Rahul Shivu Mahadev