You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Sailesh Mukil (Code Review)" <ge...@cloudera.org> on 2018/06/08 23:41:37 UTC
[Impala-ASF-CR] IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
Sailesh Mukil has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10669
Change subject: IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
......................................................................
IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
In SendReport(), if VLOG_FILE_IS_ON is 'true' (which is not the most
verbose logging level, but is higher than default), we pretty print
the profile for every fragment instance, which is a very expensive
operation, as serializing the profile is non-trivial (look at
RuntimeProfile::PrettyPrint()), and printing large amounts of information
to the logs isn't cheap as well. Lastly, it is very noisy.
This seems unnecessary since this will not benefit us, as all the
profiles are merged at the coordinator side. We could argue that this
might be necessary when an executor fails to send the profile to the
coordinator, but that signifies a network issue which will not be
reflected in the profile of any fragment instance.
This will help reduce noise in the logs when the log level is
bumped up to find other real issues that VLOG_FILE can help with.
Change-Id: Ic0445950385fa6160764feaed9a993fa0e59b242
---
M be/src/runtime/fragment-instance-state.cc
1 file changed, 0 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/10669/1
--
To view, visit http://gerrit.cloudera.org:8080/10669
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic0445950385fa6160764feaed9a993fa0e59b242
Gerrit-Change-Number: 10669
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10669 )
Change subject: IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10669
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0445950385fa6160764feaed9a993fa0e59b242
Gerrit-Change-Number: 10669
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 Jun 2018 18:17:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/10669 )
Change subject: IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10669/1/be/src/runtime/fragment-instance-state.cc
File be/src/runtime/fragment-instance-state.cc:
http://gerrit.cloudera.org:8080/#/c/10669/1/be/src/runtime/fragment-instance-state.cc@a371
PS1, Line 371:
:
:
An alternative we discussed is to bump this log statement to VLOG_ROW() or something higher. That said, it doesn't seem to provide a lot of value to print the profile of a fragment instance in an executor's log esp. in a large cluster.
Someone with more historical context of why this was done can feel free to chime in.
--
To view, visit http://gerrit.cloudera.org:8080/10669
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0445950385fa6160764feaed9a993fa0e59b242
Gerrit-Change-Number: 10669
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Jun 2018 23:59:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
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/10669 )
Change subject: IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
......................................................................
IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
In SendReport(), if VLOG_FILE_IS_ON is 'true' (which is not the most
verbose logging level, but is higher than default), we pretty print
the profile for every fragment instance, which is a very expensive
operation, as serializing the profile is non-trivial (look at
RuntimeProfile::PrettyPrint()), and printing large amounts of information
to the logs isn't cheap as well. Lastly, it is very noisy.
This seems unnecessary since this will not benefit us, as all the
profiles are merged at the coordinator side. We could argue that this
might be necessary when an executor fails to send the profile to the
coordinator, but that signifies a network issue which will not be
reflected in the profile of any fragment instance.
This will help reduce noise in the logs when the log level is
bumped up to find other real issues that VLOG_FILE can help with.
Change-Id: Ic0445950385fa6160764feaed9a993fa0e59b242
Reviewed-on: http://gerrit.cloudera.org:8080/10669
Reviewed-by: Sailesh Mukil <sa...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/runtime/fragment-instance-state.cc
1 file changed, 2 insertions(+), 7 deletions(-)
Approvals:
Sailesh Mukil: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/10669
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic0445950385fa6160764feaed9a993fa0e59b242
Gerrit-Change-Number: 10669
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10669
to look at the new patch set (#2).
Change subject: IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
......................................................................
IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
In SendReport(), if VLOG_FILE_IS_ON is 'true' (which is not the most
verbose logging level, but is higher than default), we pretty print
the profile for every fragment instance, which is a very expensive
operation, as serializing the profile is non-trivial (look at
RuntimeProfile::PrettyPrint()), and printing large amounts of information
to the logs isn't cheap as well. Lastly, it is very noisy.
This seems unnecessary since this will not benefit us, as all the
profiles are merged at the coordinator side. We could argue that this
might be necessary when an executor fails to send the profile to the
coordinator, but that signifies a network issue which will not be
reflected in the profile of any fragment instance.
This will help reduce noise in the logs when the log level is
bumped up to find other real issues that VLOG_FILE can help with.
Change-Id: Ic0445950385fa6160764feaed9a993fa0e59b242
---
M be/src/runtime/fragment-instance-state.cc
1 file changed, 2 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/10669/2
--
To view, visit http://gerrit.cloudera.org:8080/10669
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic0445950385fa6160764feaed9a993fa0e59b242
Gerrit-Change-Number: 10669
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10669 )
Change subject: IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/10669
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0445950385fa6160764feaed9a993fa0e59b242
Gerrit-Change-Number: 10669
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Jun 2018 23:58:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10669 )
Change subject: IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2643/
--
To view, visit http://gerrit.cloudera.org:8080/10669
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0445950385fa6160764feaed9a993fa0e59b242
Gerrit-Change-Number: 10669
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Jun 2018 20:33:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10669 )
Change subject: IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
......................................................................
Patch Set 1: Code-Review+1
(2 comments)
Thanks for the reviews.
Carry +1.
http://gerrit.cloudera.org:8080/#/c/10669/1/be/src/runtime/fragment-instance-state.cc
File be/src/runtime/fragment-instance-state.cc:
http://gerrit.cloudera.org:8080/#/c/10669/1/be/src/runtime/fragment-instance-state.cc@a371
PS1, Line 371:
:
:
> From git blame, it appears to be added in 368115cda. Probably was useful wh
It's actually from the following commit which is from 6 years ago:
https://github.com/apache/impala/commit/7725f25ff5219fb7440ac92d32802dd4ce3cb8f0
Commit 368115cda just moved it around. So, I feel that it's reasonable to remove this now.
http://gerrit.cloudera.org:8080/#/c/10669/1/be/src/runtime/fragment-instance-state.cc@368
PS1, Line 368: if (VLOG_FILE_IS_ON) {
> you should just remove that if-stmt now. the VLOG_FILE macro will have the
Done
--
To view, visit http://gerrit.cloudera.org:8080/10669
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0445950385fa6160764feaed9a993fa0e59b242
Gerrit-Change-Number: 10669
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 Jun 2018 18:14:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10669 )
Change subject: IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
......................................................................
Patch Set 1:
> Uploaded patch set 1.
I found this while working on another patch. If anyone feels that printing the profiles for every fragment instance periodically is useful, or has found it useful in the past, please speak up.
--
To view, visit http://gerrit.cloudera.org:8080/10669
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0445950385fa6160764feaed9a993fa0e59b242
Gerrit-Change-Number: 10669
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Jun 2018 23:43:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10669 )
Change subject: IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10669/1/be/src/runtime/fragment-instance-state.cc
File be/src/runtime/fragment-instance-state.cc:
http://gerrit.cloudera.org:8080/#/c/10669/1/be/src/runtime/fragment-instance-state.cc@368
PS1, Line 368: if (VLOG_FILE_IS_ON) {
you should just remove that if-stmt now. the VLOG_FILE macro will have the same effect.
--
To view, visit http://gerrit.cloudera.org:8080/10669
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0445950385fa6160764feaed9a993fa0e59b242
Gerrit-Change-Number: 10669
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 Jun 2018 16:25:16 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10669 )
Change subject: IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10669/1/be/src/runtime/fragment-instance-state.cc
File be/src/runtime/fragment-instance-state.cc:
http://gerrit.cloudera.org:8080/#/c/10669/1/be/src/runtime/fragment-instance-state.cc@a371
PS1, Line 371:
:
:
> An alternative we discussed is to bump this log statement to VLOG_ROW() or
From git blame, it appears to be added in 368115cda. Probably was useful when developing that change. Let's not worry about deleting it if we find it a nuisance now.
--
To view, visit http://gerrit.cloudera.org:8080/10669
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0445950385fa6160764feaed9a993fa0e59b242
Gerrit-Change-Number: 10669
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 Jun 2018 16:35:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10669 )
Change subject: IMPALA-7157: Avoid unnecessarily pretty printing profiles per fragment instance
......................................................................
Patch Set 3: Code-Review+2
Rebase, carry +2.
--
To view, visit http://gerrit.cloudera.org:8080/10669
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0445950385fa6160764feaed9a993fa0e59b242
Gerrit-Change-Number: 10669
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Jun 2018 20:33:24 +0000
Gerrit-HasComments: No