You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Ho (Code Review)" <ge...@cloudera.org> on 2018/01/24 20:18:50 UTC

[Impala-ASF-CR] KUDU-2270: Add a flag to control logging in RpczStore::LogTrace() 

Hello Kudu Jenkins, Todd Lipcon,

I'd like you to do a code review. Please visit

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

to review the following change.


Change subject: KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()
......................................................................

KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()

This change adds a new flag FLAGS_rpc_duration_too_long_ms
which controls the duration above which a RPC is considered
too long and is logged at INFO level in the log. Previously,
this threshold is hardcoded to 1000ms which may be too short
for a busy Impalad demon, leading to massive log spew.

Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
Reviewed-on: http://gerrit.cloudera.org:8080/9117
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Kudu Jenkins
---
M be/src/kudu/rpc/rpc-test.cc
M be/src/kudu/rpc/rpcz_store.cc
2 files changed, 9 insertions(+), 1 deletion(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
Gerrit-Change-Number: 9121
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[Impala-ASF-CR] KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()

Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9121 )

Change subject: KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()
......................................................................


Patch Set 2:

> Patch Set 2:
> 
> (2 comments)

Thanks for the explanations, they seem reasonable to me.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
Gerrit-Change-Number: 9121
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jan 2018 00:14:30 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has removed Todd Lipcon from this change.  ( http://gerrit.cloudera.org:8080/9121 )

Change subject: KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()
......................................................................


Removed reviewer Todd Lipcon.
-- 
To view, visit http://gerrit.cloudera.org:8080/9121
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
Gerrit-Change-Number: 9121
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>

[Impala-ASF-CR] KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9121 )

Change subject: KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()
......................................................................


Patch Set 2: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
Gerrit-Change-Number: 9121
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Jan 2018 23:24:31 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()

Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9121 )

Change subject: KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()
......................................................................


Patch Set 2: Code-Review+2

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9121/2/be/src/kudu/rpc/rpcz_store.cc
File be/src/kudu/rpc/rpcz_store.cc:

http://gerrit.cloudera.org:8080/#/c/9121/2/be/src/kudu/rpc/rpcz_store.cc@44
PS2, Line 44:              "Threshold (in milliseconds) above which a RPC is considered too long and its "
nit: The lines look too long in Gerrit.


http://gerrit.cloudera.org:8080/#/c/9121/2/be/src/kudu/rpc/rpcz_store.cc@45
PS2, Line 45: The time measured is between "
            :              "when a RPC is accepted and when its call handler completes
nit: The time is measured from when an RPC is accepted until its call handler completes.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
Gerrit-Change-Number: 9121
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Jan 2018 23:25:30 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9121 )

Change subject: KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9121/2/be/src/kudu/rpc/rpcz_store.cc
File be/src/kudu/rpc/rpcz_store.cc:

http://gerrit.cloudera.org:8080/#/c/9121/2/be/src/kudu/rpc/rpcz_store.cc@43
PS2, Line 43: DEFINE_int32_hidden
Modification from original patch. Hide this flag by default in Impala.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
Gerrit-Change-Number: 9121
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Jan 2018 23:19:42 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9121 )

Change subject: KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()
......................................................................


Patch Set 2:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
Gerrit-Change-Number: 9121
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jan 2018 01:00:06 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9121 )

Change subject: KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9121/2/be/src/kudu/rpc/rpcz_store.cc
File be/src/kudu/rpc/rpcz_store.cc:

http://gerrit.cloudera.org:8080/#/c/9121/2/be/src/kudu/rpc/rpcz_store.cc@44
PS2, Line 44:              "Threshold (in milliseconds) above which a RPC is considered too long and its "
> nit: The lines look too long in Gerrit.
Apparently, the Kudu codebase doesn't seem to limit to 90 characters per line for some reasons. Keeping it the same to avoid future conflicts.


http://gerrit.cloudera.org:8080/#/c/9121/2/be/src/kudu/rpc/rpcz_store.cc@45
PS2, Line 45: The time measured is between "
            :              "when a RPC is accepted and when its call handler completes
> nit: The time is measured from when an RPC is accepted until its call handl
Keeping it the same to avoid future conflicts.

I can push another change on Kudu side to fix up the wording if you think it's worth it.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
Gerrit-Change-Number: 9121
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Jan 2018 23:44:15 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9121 )

Change subject: KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()
......................................................................

KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()

This change adds a new flag FLAGS_rpc_duration_too_long_ms
which controls the duration above which a RPC is considered
too long and is logged at INFO level in the log. Previously,
this threshold is hardcoded to 1000ms which may be too short
for a busy Impalad demon, leading to massive log spew.

Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
Reviewed-on: http://gerrit.cloudera.org:8080/9117
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Kudu Jenkins
Reviewed-on: http://gerrit.cloudera.org:8080/9121
Reviewed-by: Lars Volker <lv...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/kudu/rpc/rpc-test.cc
M be/src/kudu/rpc/rpcz_store.cc
2 files changed, 9 insertions(+), 1 deletion(-)

Approvals:
  Todd Lipcon: Looks good to me, but someone else must approve
  Lars Volker: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
Gerrit-Change-Number: 9121
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[Impala-ASF-CR] KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9121 )

Change subject: KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9121/1/be/src/kudu/rpc/rpc-test.cc
File be/src/kudu/rpc/rpc-test.cc:

http://gerrit.cloudera.org:8080/#/c/9121/1/be/src/kudu/rpc/rpc-test.cc@46
PS1, Line 46: DECLARE_int32(rpc_duration_too_long_ms);
Not needed. Forgot to remove after the test was removed. Anyhow, keeping it for now.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
Gerrit-Change-Number: 9121
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 24 Jan 2018 20:22:22 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9121 )

Change subject: KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()
......................................................................


Patch Set 2: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
Gerrit-Change-Number: 9121
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jan 2018 04:36:05 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Kudu Jenkins, Sailesh Mukil, Todd Lipcon, 

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

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

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

Change subject: KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()
......................................................................

KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()

This change adds a new flag FLAGS_rpc_duration_too_long_ms
which controls the duration above which a RPC is considered
too long and is logged at INFO level in the log. Previously,
this threshold is hardcoded to 1000ms which may be too short
for a busy Impalad demon, leading to massive log spew.

Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
Reviewed-on: http://gerrit.cloudera.org:8080/9117
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Kudu Jenkins
---
M be/src/kudu/rpc/rpc-test.cc
M be/src/kudu/rpc/rpcz_store.cc
2 files changed, 9 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
Gerrit-Change-Number: 9121
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[Impala-ASF-CR] KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9121 )

Change subject: KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()
......................................................................


Patch Set 1:

Clean cherry-pick.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
Gerrit-Change-Number: 9121
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 24 Jan 2018 20:19:51 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has removed Kudu Jenkins from this change.  ( http://gerrit.cloudera.org:8080/9121 )

Change subject: KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()
......................................................................


Removed reviewer Kudu Jenkins.
-- 
To view, visit http://gerrit.cloudera.org:8080/9121
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
Gerrit-Change-Number: 9121
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>