You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Thomas Marshall (Code Review)" <ge...@cloudera.org> on 2018/11/21 23:14:11 UTC
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Thomas Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11977
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
IMPALA-6924: Add child queries to profile in compute stats
COMPUTE STATS triggers to child queries which do the actual stats
calculation. This patch fetches the profiles for the child queries and
adds them to the profile for the COMPUTE STATS to make it easier to
debug issues with the child queries.
Testing:
- Ran COMPUTE STATS and verified that the profile contains the
expected output both when the child queries succeed and when they
fail.
- Added a test that runs a COMPUTE STATS and checks that there are
three unique query ids in the profile.
Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
---
M be/src/service/child-query.cc
M be/src/service/child-query.h
M be/src/service/client-request-state.cc
M tests/query_test/test_observability.py
4 files changed, 46 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/11977/1
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Dec 2018 03:57:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11977/3/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:
http://gerrit.cloudera.org:8080/#/c/11977/3/common/thrift/ImpalaService.thrift@443
PS3, Line 443: 3: optional RuntimeProfile.TRuntimeProfileFormat format =
> line too long (103 > 90)
Done
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Nov 2018 23:28:49 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1485/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Nov 2018 23:25:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1490/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 01 Dec 2018 00:06:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 2:
(1 comment)
This will be very useful. I had one high level question about the approach. Otherwise the code looked good.
http://gerrit.cloudera.org:8080/#/c/11977/2/be/src/service/child-query.cc
File be/src/service/child-query.cc:
http://gerrit.cloudera.org:8080/#/c/11977/2/be/src/service/child-query.cc@104
PS2, Line 104: profile_->AddInfoString("Profile", get_profile_resp.profile);
Did you think about splicing the profiles together with AddChild(). It feels a bit wrong putting the text version of the profile in the machine-readable profile since it mixes the two formats and prevents tools from working properly.
Or is the idea that a profile analysis tool should be able to link up the queries itself if needed based on the query ids? And this is just for human convenience?
If we're going down that path maybe it would make sense to add machine-readable metadata to the thrift struct with the child query ids - it would be good to have a clear story about how a tool should use this.
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 28 Nov 2018 00:34:31 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11977
to look at the new patch set (#3).
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
IMPALA-6924: Add child queries to profile in compute stats
COMPUTE STATS triggers two child queries which do the actual stats
calculation. This patch fetches the profiles for the child queries and
adds them to the profile for the COMPUTE STATS to make it easier to
debug issues with the child queries.
To enable this, this patch also adds a 'format' parameter to
GetRuntimeProfile(), which allows clients to retrieve the profile as
either a pretty printed string (currently the only option), as a
base64 encoded string, or as a thrift structure. This allows the child
query to add the profile directly as a child of the parent query's
profile.
Note that the 'format' parameter is only available for the HiveServer2
client and not for Beeswax. This is because Thrift does not appear to
have proper support for default parameters to service methods, so
adding the 'format' parameter would not be backwards compatible with
existing Impala Beeswax clients. This does not affect HiveServer2
as it takes a 'request' struct as its only parameter and Thrift does
support default values for struct fields.
This patch also fixes a bug where '__isset' was not being set in the
Thrift runtime profile for the exec summry.
Testing:
- Ran COMPUTE STATS and verified that the profile contains the
expected output both when the child queries succeed and when they
fail.
- Added an e2e test that runs a COMPUTE STATS and checks that there
are three unique query ids in the profile.
- Added a BE test that verifies the archive string (de)serialization
functions work.
Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
---
M be/src/service/child-query.cc
M be/src/service/child-query.h
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M common/thrift/ImpalaService.thrift
M common/thrift/RuntimeProfile.thrift
M tests/query_test/test_observability.py
14 files changed, 174 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/11977/3
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 7: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 7
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 Dec 2018 00:32:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 5:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/service/client-request-state.cc@572
PS4, Line 572: child_queries.emplace_back(compute_stats_params.tbl_stats_query, this,
> I know you didn't write this line, but I see some people advocating that em
Obviously not a big deal since this isn't a super perf critical piece of code, but you're right so I went ahead and made the change.
http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:
http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/service/impala-hs2-server.cc@907
PS4, Line 907: DCHECK(request.format == TRuntimeProfileFormat::STRING || request.format == TRuntimeProfileFormat::BASE64);
> So this covers both the string and base64 cases?
Right, in the base64 case we're still just returning a string, so I'm using the same out parameter to GetRuntimeProfileOutput() 'ss' for both cases.
Obviously a bit messy/confusing but I didn't see a better way to rewrite GetRuntimeProfileOutput().
I added a DCHECK to make it clearer.
http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/util/runtime-profile-test.cc
File be/src/util/runtime-profile-test.cc:
http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/util/runtime-profile-test.cc@93
PS4, Line 93: EXPECT_TRUE(deserialized_profile->GetCounter("Not there") == nullptr);
> == nullptr
Done
http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:
http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/util/runtime-profile.cc@888
PS4, Line 888: uint32_t deserialized_len = static_cast<uint32_t>(result_len);
> Would there be any advantage to using a cast here?
My understanding is that it doesn't make a difference to how things run, but it does make the operation more explicit, which is helpful, so I went ahead and did it.
http://gerrit.cloudera.org:8080/#/c/11977/4/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:
http://gerrit.cloudera.org:8080/#/c/11977/4/tests/query_test/test_observability.py@265
PS4, Line 265: # Search for all query ids (max length 33) in the profile.
> So 33 is the length of a query id? That made me think.
Added a comment
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Dec 2018 19:04:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 4: Code-Review+1
(5 comments)
This all looks good to me. I have comments that are more like questions so I can learn things.
http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/service/client-request-state.cc@572
PS4, Line 572: child_queries.push_back(ChildQuery(compute_stats_params.tbl_stats_query, this,
I know you didn't write this line, but I see some people advocating that emplace_back is better than push_back as it can avoid a copy?
http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:
http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/service/impala-hs2-server.cc@907
PS4, Line 907: return_val.__set_profile(ss.str());
So this covers both the string and base64 cases?
http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/util/runtime-profile-test.cc
File be/src/util/runtime-profile-test.cc:
http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/util/runtime-profile-test.cc@93
PS4, Line 93: EXPECT_TRUE(deserialized_profile->GetCounter("Not there") == NULL);
== nullptr
http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:
http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/util/runtime-profile.cc@888
PS4, Line 888: uint32_t deserialized_len = result_len;
Would there be any advantage to using a cast here?
http://gerrit.cloudera.org:8080/#/c/11977/4/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:
http://gerrit.cloudera.org:8080/#/c/11977/4/tests/query_test/test_observability.py@265
PS4, Line 265: matches = re.findall("Query \(id=.{,33}\)", results.runtime_profile)
So 33 is the length of a query id? That made me think.
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Dec 2018 17:41:49 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11977/3/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:
http://gerrit.cloudera.org:8080/#/c/11977/3/common/thrift/ImpalaService.thrift@443
PS3, Line 443: 3: optional RuntimeProfile.TRuntimeProfileFormat format = RuntimeProfile.TRuntimeProfileFormat.STRING
line too long (103 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Nov 2018 23:00:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3522/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 7
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Dec 2018 20:43:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11977/5/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:
http://gerrit.cloudera.org:8080/#/c/11977/5/be/src/service/impala-hs2-server.cc@907
PS5, Line 907: DCHECK(request.format == TRuntimeProfileFormat::STRING
> line too long (111 > 90)
Done
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Dec 2018 19:55:31 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11977
to look at the new patch set (#2).
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
IMPALA-6924: Add child queries to profile in compute stats
COMPUTE STATS triggers to child queries which do the actual stats
calculation. This patch fetches the profiles for the child queries and
adds them to the profile for the COMPUTE STATS to make it easier to
debug issues with the child queries.
Testing:
- Ran COMPUTE STATS and verified that the profile contains the
expected output both when the child queries succeed and when they
fail.
- Added a test that runs a COMPUTE STATS and checks that there are
three unique query ids in the profile.
Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
---
M be/src/service/child-query.cc
M be/src/service/child-query.h
M be/src/service/client-request-state.cc
M tests/query_test/test_observability.py
4 files changed, 51 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/11977/2
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11977/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11977/2//COMMIT_MSG@9
PS2, Line 9: COMPUTE STATS triggers two child queries which do the actual stats
> Should this be "triggers some child queries"?
Done
http://gerrit.cloudera.org:8080/#/c/11977/2/be/src/service/child-query.h
File be/src/service/child-query.h:
http://gerrit.cloudera.org:8080/#/c/11977/2/be/src/service/child-query.h@129
PS2, Line 129: ImpalaServer* parent_server_;
> Do you want a /// comment here?
Done
http://gerrit.cloudera.org:8080/#/c/11977/2/be/src/service/child-query.cc
File be/src/service/child-query.cc:
http://gerrit.cloudera.org:8080/#/c/11977/2/be/src/service/child-query.cc@104
PS2, Line 104: is_running_ = false;
> You're right. I think if we wanted to go that way it might be not that bad
I think having the mode flag to GetRuntimeProfile is a nice addition, so I went ahead and did it.
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Nov 2018 22:59:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11977/1/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/11977/1/be/src/service/client-request-state.cc@563
PS1, Line 563: RuntimeProfile* child_profile = RuntimeProfile::Create(&profile_pool_, "Child Queries");
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/11977/1/be/src/service/client-request-state.cc@568
PS1, Line 568: RuntimeProfile* profile = RuntimeProfile::Create(&profile_pool_, "Table Stats Query");
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/11977/1/be/src/service/client-request-state.cc@571
PS1, Line 571: ChildQuery(compute_stats_params.tbl_stats_query, this, parent_server_, profile));
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/11977/1/be/src/service/client-request-state.cc@574
PS1, Line 574: RuntimeProfile* profile = RuntimeProfile::Create(&profile_pool_, "Column Stats Query");
line too long (93 > 90)
http://gerrit.cloudera.org:8080/#/c/11977/1/be/src/service/client-request-state.cc@577
PS1, Line 577: ChildQuery(compute_stats_params.col_stats_query, this, parent_server_, profile));
line too long (91 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Nov 2018 23:14:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11977
to look at the new patch set (#4).
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
IMPALA-6924: Add child queries to profile in compute stats
COMPUTE STATS triggers two child queries which do the actual stats
calculation. This patch fetches the profiles for the child queries and
adds them to the profile for the COMPUTE STATS to make it easier to
debug issues with the child queries.
To enable this, this patch also adds a 'format' parameter to
GetRuntimeProfile(), which allows clients to retrieve the profile as
either a pretty printed string (currently the only option), as a
base64 encoded string, or as a thrift structure. This allows the child
query to add the profile directly as a child of the parent query's
profile.
Note that the 'format' parameter is only available for the HiveServer2
client and not for Beeswax. This is because Thrift does not appear to
have proper support for default parameters to service methods, so
adding the 'format' parameter would not be backwards compatible with
existing Impala Beeswax clients. This does not affect HiveServer2
as it takes a 'request' struct as its only parameter and Thrift does
support default values for struct fields.
This patch also fixes a bug where '__isset' was not being set in the
Thrift runtime profile for the exec summry.
Testing:
- Ran COMPUTE STATS and verified that the profile contains the
expected output both when the child queries succeed and when they
fail.
- Added an e2e test that runs a COMPUTE STATS and checks that there
are three unique query ids in the profile.
- Added a BE test that verifies the archive string (de)serialization
functions work.
Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
---
M be/src/service/child-query.cc
M be/src/service/child-query.h
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M common/thrift/ImpalaService.thrift
M common/thrift/RuntimeProfile.thrift
M tests/query_test/test_observability.py
14 files changed, 175 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/11977/4
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
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/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
IMPALA-6924: Add child queries to profile in compute stats
COMPUTE STATS triggers two child queries which do the actual stats
calculation. This patch fetches the profiles for the child queries and
adds them to the profile for the COMPUTE STATS to make it easier to
debug issues with the child queries.
To enable this, this patch also adds a 'format' parameter to
GetRuntimeProfile(), which allows clients to retrieve the profile as
either a pretty printed string (currently the only option), as a
base64 encoded string, or as a thrift structure. This allows the child
query to add the profile directly as a child of the parent query's
profile.
Note that the 'format' parameter is only available for the HiveServer2
client and not for Beeswax. This is because Thrift does not appear to
have proper support for default parameters to service methods, so
adding the 'format' parameter would not be backwards compatible with
existing Impala Beeswax clients. This does not affect HiveServer2
as it takes a 'request' struct as its only parameter and Thrift does
support default values for struct fields.
This patch also fixes a bug where '__isset' was not being set in the
Thrift runtime profile for the exec summry.
Testing:
- Ran COMPUTE STATS and verified that the profile contains the
expected output both when the child queries succeed and when they
fail.
- Added an e2e test that runs a COMPUTE STATS and checks that there
are three unique query ids in the profile.
- Added a BE test that verifies the archive string (de)serialization
functions work.
Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Reviewed-on: http://gerrit.cloudera.org:8080/11977
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/service/child-query.cc
M be/src/service/child-query.h
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M common/thrift/ImpalaService.thrift
M common/thrift/RuntimeProfile.thrift
M tests/query_test/test_observability.py
14 files changed, 178 insertions(+), 20 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 8
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11977
to look at the new patch set (#5).
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
IMPALA-6924: Add child queries to profile in compute stats
COMPUTE STATS triggers two child queries which do the actual stats
calculation. This patch fetches the profiles for the child queries and
adds them to the profile for the COMPUTE STATS to make it easier to
debug issues with the child queries.
To enable this, this patch also adds a 'format' parameter to
GetRuntimeProfile(), which allows clients to retrieve the profile as
either a pretty printed string (currently the only option), as a
base64 encoded string, or as a thrift structure. This allows the child
query to add the profile directly as a child of the parent query's
profile.
Note that the 'format' parameter is only available for the HiveServer2
client and not for Beeswax. This is because Thrift does not appear to
have proper support for default parameters to service methods, so
adding the 'format' parameter would not be backwards compatible with
existing Impala Beeswax clients. This does not affect HiveServer2
as it takes a 'request' struct as its only parameter and Thrift does
support default values for struct fields.
This patch also fixes a bug where '__isset' was not being set in the
Thrift runtime profile for the exec summry.
Testing:
- Ran COMPUTE STATS and verified that the profile contains the
expected output both when the child queries succeed and when they
fail.
- Added an e2e test that runs a COMPUTE STATS and checks that there
are three unique query ids in the profile.
- Added a BE test that verifies the archive string (de)serialization
functions work.
Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
---
M be/src/service/child-query.cc
M be/src/service/child-query.h
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M common/thrift/ImpalaService.thrift
M common/thrift/RuntimeProfile.thrift
M tests/query_test/test_observability.py
14 files changed, 177 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/11977/5
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1426/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Wed, 21 Nov 2018 23:44:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11977/2/be/src/service/child-query.cc
File be/src/service/child-query.cc:
http://gerrit.cloudera.org:8080/#/c/11977/2/be/src/service/child-query.cc@104
PS2, Line 104: profile_->AddInfoString("Profile", get_profile_resp.profile);
> Did you think about splicing the profiles together with AddChild(). It feel
Do we expose any way to get the actual Thrift profile with a client? I thought we only exposed it as a string, eg. that's what GetRuntimeProfile() returns
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Nov 2018 00:27:44 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Dec 2018 19:57:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11977
to look at the new patch set (#6).
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
IMPALA-6924: Add child queries to profile in compute stats
COMPUTE STATS triggers two child queries which do the actual stats
calculation. This patch fetches the profiles for the child queries and
adds them to the profile for the COMPUTE STATS to make it easier to
debug issues with the child queries.
To enable this, this patch also adds a 'format' parameter to
GetRuntimeProfile(), which allows clients to retrieve the profile as
either a pretty printed string (currently the only option), as a
base64 encoded string, or as a thrift structure. This allows the child
query to add the profile directly as a child of the parent query's
profile.
Note that the 'format' parameter is only available for the HiveServer2
client and not for Beeswax. This is because Thrift does not appear to
have proper support for default parameters to service methods, so
adding the 'format' parameter would not be backwards compatible with
existing Impala Beeswax clients. This does not affect HiveServer2
as it takes a 'request' struct as its only parameter and Thrift does
support default values for struct fields.
This patch also fixes a bug where '__isset' was not being set in the
Thrift runtime profile for the exec summry.
Testing:
- Ran COMPUTE STATS and verified that the profile contains the
expected output both when the child queries succeed and when they
fail.
- Added an e2e test that runs a COMPUTE STATS and checks that there
are three unique query ids in the profile.
- Added a BE test that verifies the archive string (de)serialization
functions work.
Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
---
M be/src/service/child-query.cc
M be/src/service/child-query.h
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M common/thrift/ImpalaService.thrift
M common/thrift/RuntimeProfile.thrift
M tests/query_test/test_observability.py
14 files changed, 178 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/11977/6
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1427/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Wed, 21 Nov 2018 23:55:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 2:
(2 comments)
Looks like a useful change
http://gerrit.cloudera.org:8080/#/c/11977/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11977/2//COMMIT_MSG@9
PS2, Line 9: COMPUTE STATS triggers to child queries which do the actual stats
Should this be "triggers some child queries"?
Or "triggers two child queries"?
http://gerrit.cloudera.org:8080/#/c/11977/2/be/src/service/child-query.h
File be/src/service/child-query.h:
http://gerrit.cloudera.org:8080/#/c/11977/2/be/src/service/child-query.h@129
PS2, Line 129: RuntimeProfile* profile_;
Do you want a /// comment here?
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Nov 2018 01:07:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11977/5/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:
http://gerrit.cloudera.org:8080/#/c/11977/5/be/src/service/impala-hs2-server.cc@907
PS5, Line 907: DCHECK(request.format == TRuntimeProfileFormat::STRING || request.format == TRuntimeProfileFormat::BASE64);
line too long (111 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Dec 2018 19:04:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1532/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Dec 2018 19:36:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11977/1/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/11977/1/be/src/service/client-request-state.cc@563
PS1, Line 563: RuntimeProfile* child_profile =
> line too long (92 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/11977/1/be/src/service/client-request-state.cc@568
PS1, Line 568: if (compute_stats_params.__isset.tbl_stats_query) {
> line too long (92 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/11977/1/be/src/service/client-request-state.cc@571
PS1, Line 571: child_profile->AddChild(profile);
> line too long (91 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/11977/1/be/src/service/client-request-state.cc@574
PS1, Line 574: }
> line too long (93 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/11977/1/be/src/service/client-request-state.cc@577
PS1, Line 577: RuntimeProfile::Create(&profile_pool_, "Column Stats Query");
> line too long (91 > 90)
Done
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Wed, 21 Nov 2018 23:16:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1533/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Dec 2018 20:30:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 7
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Dec 2018 20:43:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6924: Add child queries to profile in compute stats
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11977/2/be/src/service/child-query.cc
File be/src/service/child-query.cc:
http://gerrit.cloudera.org:8080/#/c/11977/2/be/src/service/child-query.cc@104
PS2, Line 104: profile_->AddInfoString("Profile", get_profile_resp.profile);
> Do we expose any way to get the actual Thrift profile with a client? I thou
You're right. I think if we wanted to go that way it might be not that bad to plumb through - we could add an optional mode flag in TRuntimeProfileRequest and an optional TRuntimeProfileTree to TRuntimeProfileResponse.
Or we could add a new method to ImpalaServer directly.
Anyway, that's only relevant if we want to go down that path. I don't want the perfect to be the enemy of the useful here.
--
To view, visit http://gerrit.cloudera.org:8080/11977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350
Gerrit-Change-Number: 11977
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Nov 2018 01:38:22 +0000
Gerrit-HasComments: Yes