You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Kurt Deschler (Code Review)" <ge...@cloudera.org> on 2023/08/18 14:34:03 UTC

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

Kurt Deschler has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20377


Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................

IMPALA-12385: Enable Periodic metrics by default

This patch enables periodic metrics in query profiles by default and
changes the metric collectors to be more suitable for mixed workloads.

-Change default of resource_trace_ratio to 1
-Use samplng counters which can automatically resize for long queries
-Reduce metric interval to 50ms to support short-running queries
-Fragment metrics use the same sample interval as periodic metrics

Testing:
  Updated runtime-profile-test and test_observability.py for new defaults
  Manual inspection of query profile metrics for long-running queries

Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
---
M be/src/runtime/query-state.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/streaming-sampler.h
M common/thrift/Query.thrift
M tests/query_test/test_observability.py
8 files changed, 34 insertions(+), 31 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 1
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 3:

(5 comments)

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

http://gerrit.cloudera.org:8080/#/c/20377/1//COMMIT_MSG@12
PS1, Line 12: resource_trace_ratio to 1
> I didn't see any significant overhead, even with sampling at 10ms. Can you 
Done


http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/runtime/query-state.cc@221
PS1, Line 221: AddSamplingTimeSeriesCounter
> The code appears to handle this already. Note that SamplingTimeSeriesCounte
Done


http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/util/periodic-counter-updater.cc
File be/src/util/periodic-counter-updater.cc:

http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/util/periodic-counter-updater.cc@30
PS1, Line 30: periodic_counter_update_period_ms, 50
> 50ms doesn't appear to create performance issues with single-user queries. 
Done


http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:

http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/util/runtime-profile-counters.h@807
PS1, Line 807: typedef StreamingSampler<int64_t, 64> StreamingCounterSampler;
> Queries on the order of 1sec were not affected. I will test more with short
Done


http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/util/streaming-sampler.h
File be/src/util/streaming-sampler.h:

http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/util/streaming-sampler.h@40
PS1, Line 40: int initial_period
> Memory and thread usage need to use the lower interval to short-running que
This still need to get addressed, at least for DeferredQueueSize.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 3
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Aug 2023 23:37:40 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 1:

(5 comments)

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

http://gerrit.cloudera.org:8080/#/c/20377/1//COMMIT_MSG@12
PS1, Line 12: resource_trace_ratio to 1
> https://issues.apache.org/jira/browse/IMPALA-7694 seems to posit that it mi
Riza had the same concern so I did additional testing. We will re-visit if any problems are observed.


http://gerrit.cloudera.org:8080/#/c/20377/1//COMMIT_MSG@13
PS1, Line 13: -Use samplng counters which can automatically resize for long queries
> nit: type in samplng
Done


http://gerrit.cloudera.org:8080/#/c/20377/1//COMMIT_MSG@14
PS1, Line 14: -Reduce metric interval to 50ms to support short-running queries
> Can you add an example of how this shows up in a Profile?
Done


http://gerrit.cloudera.org:8080/#/c/20377/6/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:

http://gerrit.cloudera.org:8080/#/c/20377/6/be/src/util/runtime-profile-counters.h@822
PS6, Line 822: };
> nit: this formatting doesn't seem consistent with our usual style.
Done


http://gerrit.cloudera.org:8080/#/c/20377/6/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:

http://gerrit.cloudera.org:8080/#/c/20377/6/tests/query_test/test_observability.py@549
PS6, Line 549:       # We check for 500ms because a query with 1s duration won't hit the 64 values limit
> Comment should say 50ms now.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 1
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Sat, 09 Sep 2023 12:10:08 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Hello Riza Suminto, David Rorke, Surya Hebbar, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................

IMPALA-12385: Enable Periodic metrics by default

This patch enables periodic metrics in query profiles by default and
changes the metric collectors to be more suitable for mixed workloads.

-Change default of resource_trace_ratio to 1
-Use samplng counters which can automatically resize for long queries
-Reduce metric interval to 50ms to support short-running queries
-Fragment metrics use the same sample interval as periodic metrics

Testing:
-Updated runtime-profile-test and test_observability.py for new defaults
-Manual inspection of query profile metrics for long-running queries
-Tested for performance regression using 50 concurrent TPCH Q1 and with
 periodic_counter_update_period_ms=1 to verify that the periodic update
 loop does not affect peformance or become a bottleneck

Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
---
M be/src/runtime/query-state.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/streaming-sampler.h
M common/thrift/Query.thrift
M tests/query_test/test_observability.py
8 files changed, 35 insertions(+), 31 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 3
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 5:

(5 comments)

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

http://gerrit.cloudera.org:8080/#/c/20377/5//COMMIT_MSG@15
PS5, Line 15: -Added new flag periodic_system_counter_update_period_ms and additional
            :  update thread to preserve 500ms update interval for KRPC metrics
> Mention the new separate system_instance_ singleton in commit message.
Done


http://gerrit.cloudera.org:8080/#/c/20377/5/be/src/util/periodic-counter-updater.h
File be/src/util/periodic-counter-updater.h:

http://gerrit.cloudera.org:8080/#/c/20377/5/be/src/util/periodic-counter-updater.h@158
PS5, Line 158: /// Singleton object that keeps track of all profile rate counters and the thread
             :   /// for updating them.
> nit: mention the flag or the default sampling period for this singleton.
Done


http://gerrit.cloudera.org:8080/#/c/20377/5/be/src/util/periodic-counter-updater.h@162
PS5, Line 162: /// Singleton object that keeps track of all system rate counters and the thread
             :   /// for updating them.
> nit: mention the flag or the default sampling period for this singleton.
Done


http://gerrit.cloudera.org:8080/#/c/20377/5/be/src/util/periodic-counter-updater.cc
File be/src/util/periodic-counter-updater.cc:

http://gerrit.cloudera.org:8080/#/c/20377/5/be/src/util/periodic-counter-updater.cc@47
PS5, Line 47: the singleton
> nit: two singleton
Done


http://gerrit.cloudera.org:8080/#/c/20377/5/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:

http://gerrit.cloudera.org:8080/#/c/20377/5/be/src/util/runtime-profile.h@156
PS5, Line 156: bool is_system_;
> Looks like is_system_ is always false for RuntimeProfile::Counter. Is it OK
I actually missed some counters that were instantiated through macros. So KRPC does have time series counters that need this logic.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 5
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Aug 2023 17:10:23 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 6: Code-Review+1

Putting +1 since I don't mind it merge as is.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 6
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Aug 2023 18:53:07 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 6:

(5 comments)

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

http://gerrit.cloudera.org:8080/#/c/20377/1//COMMIT_MSG@12
PS1, Line 12:  resource_trace_ratio to 
> Done
https://issues.apache.org/jira/browse/IMPALA-7694 seems to posit that it might be a problem, but doesn't provide any support that it is. They seem to have just assumed it would be and made it disabled by default.


http://gerrit.cloudera.org:8080/#/c/20377/1//COMMIT_MSG@13
PS1, Line 13: -Changed profile metrics to use samplng counters which can automatically
nit: type in samplng


http://gerrit.cloudera.org:8080/#/c/20377/1//COMMIT_MSG@14
PS1, Line 14:  resize for long queries.
Can you add an example of how this shows up in a Profile?


http://gerrit.cloudera.org:8080/#/c/20377/6/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:

http://gerrit.cloudera.org:8080/#/c/20377/6/be/src/util/runtime-profile-counters.h@822
PS6, Line 822:     , samples_(initial_period) {}
nit: this formatting doesn't seem consistent with our usual style.


http://gerrit.cloudera.org:8080/#/c/20377/6/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:

http://gerrit.cloudera.org:8080/#/c/20377/6/tests/query_test/test_observability.py@549
PS6, Line 549:       # We check for 500ms because a query with 1s duration won't hit the 64 values limit
Comment should say 50ms now.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 6
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Sep 2023 21:18:40 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 10: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 10
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Dec 2023 18:22:38 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 10: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 10
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Dec 2023 22:51:50 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 1:

(5 comments)

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

http://gerrit.cloudera.org:8080/#/c/20377/1//COMMIT_MSG@12
PS1, Line 12: resource_trace_ratio to 1
> AFAIK, there is a pretty significant overhead on always sampling this metri
I didn't see any significant overhead, even with sampling at 10ms. Can you please provide an examples of a query that is slower?


http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/runtime/query-state.cc@221
PS1, Line 221: AddSamplingTimeSeriesCounter
> Will this cause interpretation problem if different host happen to resize i
The code appears to handle this already. Note that SamplingTimeSeriesCounter is already being used for Fragment metrics.


http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/util/periodic-counter-updater.cc
File be/src/util/periodic-counter-updater.cc:

http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/util/periodic-counter-updater.cc@30
PS1, Line 30: periodic_counter_update_period_ms, 50
> I'm a bit concern about lowering this to 10x. Can the code in PeriodicCount
50ms doesn't appear to create performance issues with single-user queries. I will test with concurrent queries. Even at 100ms, values are too far apart for detailed analysis of short queries.


http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:

http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/util/runtime-profile-counters.h@807
PS1, Line 807: typedef StreamingSampler<int64_t, 64> StreamingCounterSampler;
> If initial_period = 50ms, and MAX_SAMPLES = 64, that means it will take 320
Queries on the order of 1sec were not affected. I will test more with shorter queries.


http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/util/streaming-sampler.h
File be/src/util/streaming-sampler.h:

http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/util/streaming-sampler.h@40
PS1, Line 40: int initial_period
> I'd rather keep this default to 500, but then add new parameter in AddSampl
Memory and thread usage need to use the lower interval to short-running queries. I can understand adding a different switch to preserve the 500ms default for KRPC.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 1
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Aug 2023 22:11:07 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Hello Riza Suminto, David Rorke, Surya Hebbar, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................

IMPALA-12385: Enable Periodic metrics by default

This patch enables periodic metrics in query profiles by default and
changes the metric collectors to be more suitable for mixed workloads.

-Change default of resource_trace_ratio to 1
-Use samplng counters which can automatically resize for long queries
-Reduce metric interval to 50ms to support short-running queries
-Added new flag periodic_system_counter_update_period_ms and additional
 update thread to preserve 500ms update interval for KRPC metrics
-Fragment metrics use the same sample interval as periodic metrics

Testing:
-Updated runtime-profile-test and test_observability.py for new defaults
-Manual inspection of query profile metrics for long-running queries
-Tested for performance regression using 50 concurrent TPCH Q1 and with
 periodic_counter_update_period_ms=1 to verify that the periodic update
 loop does not affect peformance or become a bottleneck

Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
---
M be/src/runtime/krpc-data-stream-recvr.cc
M be/src/runtime/query-state.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M be/src/util/streaming-sampler.h
M common/thrift/Query.thrift
M tests/query_test/test_observability.py
11 files changed, 75 insertions(+), 43 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 4
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/20377/4/be/src/util/periodic-counter-updater.cc
File be/src/util/periodic-counter-updater.cc:

http://gerrit.cloudera.org:8080/#/c/20377/4/be/src/util/periodic-counter-updater.cc@136
PS4, Line 136:   (is_system_counter ? system_instance_ : instance_)->time_series_counters_.insert(counter);
line too long (92 > 90)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 4
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Wed, 30 Aug 2023 03:56:27 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:

http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/util/runtime-profile.cc@2023
PS1, Line 2023:   TimeSeriesCounter* counter = pool_->Add(new SamplingTimeSeriesCounter(name, unit, fn, FLAGS_periodic_counter_update_period_ms));
line too long (130 > 90)


http://gerrit.cloudera.org:8080/#/c/20377/1/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:

http://gerrit.cloudera.org:8080/#/c/20377/1/tests/query_test/test_observability.py@532
PS1, Line 532: r
flake8: W291 trailing whitespace


http://gerrit.cloudera.org:8080/#/c/20377/1/tests/query_test/test_observability.py@532
PS1, Line 532:     """Tests that the query profile does not contain resource usage metrics 
line has trailing whitespace



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 1
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Aug 2023 14:35:07 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/13779/ : 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/20377
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 2
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Aug 2023 15:50:33 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/20377 )

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................

IMPALA-12385: Enable Periodic metrics by default

This patch enables periodic metrics in query profiles by default and
changes the metric collectors to be more suitable for mixed workloads.

-Changed default of resource_trace_ratio to 1.
-Changed profile metrics to use sampling counters which can automatically
 resize for long queries.
-Reduced profile metric samping interval to 50ms to support short-running
 queries.
-Changed fragment metrics to use the same sampling interval as periodic
 metrics.
-Added singleton PeriodicCounterUpdater and thread for updating system
 (KRPC) metrics at a different period than fragment metrics.
-Added new flag periodic_system_counter_update_period_ms for configuring
 system metric update period. Default is 500ms.

Example profile output:
- HostCpuIoWaitPercentage (400.000ms): 1, 0, 2, 3, 4, 6, 5, 2, 1, ...
- HostCpuSysPercentage (400.000ms): 1, 12, 10, 11, 11, 5, 3, 3, 3, ...
- HostCpuUserPercentage (400.000ms): 8, 46, 39, 39, 39, 29, 22, 23, ...

Testing:
-Updated runtime-profile-test and test_observability.py for new defaults
-Manual inspection of query profile metrics for long-running queries
-Tested for performance regression using 50 concurrent TPCH Q1 and with
 periodic_counter_update_period_ms=1 to verify that the periodic update
 loop does not affect peformance or become a bottleneck

Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Reviewed-on: http://gerrit.cloudera.org:8080/20377
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Michael Smith <mi...@cloudera.com>
---
M be/src/runtime/exec-env.cc
M be/src/runtime/krpc-data-stream-recvr.cc
M be/src/runtime/query-state.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M be/src/util/streaming-sampler.h
M common/thrift/Query.thrift
M tests/query_test/test_observability.py
12 files changed, 147 insertions(+), 81 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Michael Smith: Looks good to me, approved

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 11
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Hello Riza Suminto, David Rorke, Surya Hebbar, Csaba Ringhofer, Michael Smith, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................

IMPALA-12385: Enable Periodic metrics by default

This patch enables periodic metrics in query profiles by default and
changes the metric collectors to be more suitable for mixed workloads.

-Changed default of resource_trace_ratio to 1.
-Changed profile metrics to use sampling counters which can automatically
 resize for long queries.
-Reduced profile metric samping interval to 50ms to support short-running
 queries.
-Changed fragment metrics to use the same sampling interval as periodic
 metrics.
-Added singleton PeriodicCounterUpdater and thread for updating system
 (KRPC) metrics at a different period than fragment metrics.
-Added new flag periodic_system_counter_update_period_ms for configuring
 system metric update period. Default is 500ms.

Example profile output:
- HostCpuIoWaitPercentage (400.000ms): 1, 0, 2, 3, 4, 6, 5, 2, 1, ...
- HostCpuSysPercentage (400.000ms): 1, 12, 10, 11, 11, 5, 3, 3, 3, ...
- HostCpuUserPercentage (400.000ms): 8, 46, 39, 39, 39, 29, 22, 23, ...

Testing:
-Updated runtime-profile-test and test_observability.py for new defaults
-Manual inspection of query profile metrics for long-running queries
-Tested for performance regression using 50 concurrent TPCH Q1 and with
 periodic_counter_update_period_ms=1 to verify that the periodic update
 loop does not affect peformance or become a bottleneck

Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
---
M be/src/runtime/exec-env.cc
M be/src/runtime/krpc-data-stream-recvr.cc
M be/src/runtime/query-state.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M be/src/util/streaming-sampler.h
M common/thrift/Query.thrift
M tests/query_test/test_observability.py
12 files changed, 147 insertions(+), 81 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/20377/8
-- 
To view, visit http://gerrit.cloudera.org:8080/20377
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 8
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 9: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/9699/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 9
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 Sep 2023 22:07:53 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Hello Riza Suminto, David Rorke, Surya Hebbar, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................

IMPALA-12385: Enable Periodic metrics by default

This patch enables periodic metrics in query profiles by default and
changes the metric collectors to be more suitable for mixed workloads.

-Change default of resource_trace_ratio to 1
-Use samplng counters which can automatically resize for long queries
-Reduce metric interval to 50ms to support short-running queries
-Added new flag periodic_system_counter_update_period_ms and additional
 update thread to preserve 500ms update interval for KRPC metrics
-Fragment metrics use the same sample interval as periodic metrics

Testing:
-Updated runtime-profile-test and test_observability.py for new defaults
-Manual inspection of query profile metrics for long-running queries
-Tested for performance regression using 50 concurrent TPCH Q1 and with
 periodic_counter_update_period_ms=1 to verify that the periodic update
 loop does not affect peformance or become a bottleneck

Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
---
M be/src/runtime/exec-env.cc
M be/src/runtime/krpc-data-stream-recvr.cc
M be/src/runtime/query-state.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M be/src/util/streaming-sampler.h
M common/thrift/Query.thrift
M tests/query_test/test_observability.py
12 files changed, 139 insertions(+), 74 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 5
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 5:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/13889/ : 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/20377
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 5
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Wed, 30 Aug 2023 16:54:45 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Hello Riza Suminto, David Rorke, Surya Hebbar, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................

IMPALA-12385: Enable Periodic metrics by default

This patch enables periodic metrics in query profiles by default and
changes the metric collectors to be more suitable for mixed workloads.

-Changed default of resource_trace_ratio to 1.
-Changed profile metrics to use samplng counters which can automatically
 resize for long queries.
-Reduced profile metric samping interval to 50ms to support short-running
 queries.
-Changed fragment metrics to use the same sampling interval as periodic
 metrics.
-Added singleton PeriodicCounterUpdater and thread for updating system
 (KRPC) metrics at a different period than fragment metrics.
-Added new flag periodic_system_counter_update_period_ms for configuring
 system metric update period. Default is 500ms.

Testing:
-Updated runtime-profile-test and test_observability.py for new defaults
-Manual inspection of query profile metrics for long-running queries
-Tested for performance regression using 50 concurrent TPCH Q1 and with
 periodic_counter_update_period_ms=1 to verify that the periodic update
 loop does not affect peformance or become a bottleneck

Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
---
M be/src/runtime/exec-env.cc
M be/src/runtime/krpc-data-stream-recvr.cc
M be/src/runtime/query-state.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M be/src/util/streaming-sampler.h
M common/thrift/Query.thrift
M tests/query_test/test_observability.py
12 files changed, 148 insertions(+), 81 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/20377/6
-- 
To view, visit http://gerrit.cloudera.org:8080/20377
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 6
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 10:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/14595/ : 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/20377
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 10
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Dec 2023 04:16:45 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 10: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 10
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Dec 2023 22:58:14 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 10:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9984/ DRY_RUN=true


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 10
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Dec 2023 18:22:12 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 8:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/13966/ : 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/20377
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 8
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Sat, 09 Sep 2023 12:37:27 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Hello Riza Suminto, David Rorke, Surya Hebbar, Csaba Ringhofer, Michael Smith, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................

IMPALA-12385: Enable Periodic metrics by default

This patch enables periodic metrics in query profiles by default and
changes the metric collectors to be more suitable for mixed workloads.

-Changed default of resource_trace_ratio to 1.
-Changed profile metrics to use sampling counters which can automatically
 resize for long queries.
-Reduced profile metric samping interval to 50ms to support short-running
 queries.
-Changed fragment metrics to use the same sampling interval as periodic
 metrics.
-Added singleton PeriodicCounterUpdater and thread for updating system
 (KRPC) metrics at a different period than fragment metrics.
-Added new flag periodic_system_counter_update_period_ms for configuring
 system metric update period. Default is 500ms.

Example profile output:
- HostCpuIoWaitPercentage (400.000ms): 1, 0, 2, 3, 4, 6, 5, 2, 1, ...
- HostCpuSysPercentage (400.000ms): 1, 12, 10, 11, 11, 5, 3, 3, 3, ...
- HostCpuUserPercentage (400.000ms): 8, 46, 39, 39, 39, 29, 22, 23, ...

Testing:
-Updated runtime-profile-test and test_observability.py for new defaults
-Manual inspection of query profile metrics for long-running queries
-Tested for performance regression using 50 concurrent TPCH Q1 and with
 periodic_counter_update_period_ms=1 to verify that the periodic update
 loop does not affect peformance or become a bottleneck

Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
---
M be/src/runtime/exec-env.cc
M be/src/runtime/krpc-data-stream-recvr.cc
M be/src/runtime/query-state.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M be/src/util/streaming-sampler.h
M common/thrift/Query.thrift
M tests/query_test/test_observability.py
12 files changed, 147 insertions(+), 81 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/20377/7
-- 
To view, visit http://gerrit.cloudera.org:8080/20377
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 7
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 9: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 9
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 Sep 2023 17:45:33 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 6:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/13898/ : 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/20377
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 6
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Aug 2023 16:33:00 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 5:

(5 comments)

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

http://gerrit.cloudera.org:8080/#/c/20377/5//COMMIT_MSG@15
PS5, Line 15: -Added new flag periodic_system_counter_update_period_ms and additional
            :  update thread to preserve 500ms update interval for KRPC metrics
Mention the new separate system_instance_ singleton in commit message.


http://gerrit.cloudera.org:8080/#/c/20377/5/be/src/util/periodic-counter-updater.h
File be/src/util/periodic-counter-updater.h:

http://gerrit.cloudera.org:8080/#/c/20377/5/be/src/util/periodic-counter-updater.h@158
PS5, Line 158: /// Singleton object that keeps track of all profile rate counters and the thread
             :   /// for updating them.
nit: mention the flag or the default sampling period for this singleton.


http://gerrit.cloudera.org:8080/#/c/20377/5/be/src/util/periodic-counter-updater.h@162
PS5, Line 162: /// Singleton object that keeps track of all system rate counters and the thread
             :   /// for updating them.
nit: mention the flag or the default sampling period for this singleton.


http://gerrit.cloudera.org:8080/#/c/20377/5/be/src/util/periodic-counter-updater.cc
File be/src/util/periodic-counter-updater.cc:

http://gerrit.cloudera.org:8080/#/c/20377/5/be/src/util/periodic-counter-updater.cc@47
PS5, Line 47: the singleton
nit: two singleton


http://gerrit.cloudera.org:8080/#/c/20377/5/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:

http://gerrit.cloudera.org:8080/#/c/20377/5/be/src/util/runtime-profile.h@156
PS5, Line 156: bool is_system_;
Looks like is_system_ is always false for RuntimeProfile::Counter. Is it OK to remove this and assume all RuntimeProfile::Counter is non-system in periodic-counter-updater.cc?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 5
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Wed, 30 Aug 2023 17:52:12 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/20377/5/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:

http://gerrit.cloudera.org:8080/#/c/20377/5/be/src/util/runtime-profile.h@156
PS5, Line 156: bool is_system_;
> nit: I still think that this can be removed because RuntimeProfile::Counter
I'm going to leave it in cases we want to use that combo in the future and so there are no hard references to instance_.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 5
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Aug 2023 23:04:07 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/13778/ : 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/20377
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 1
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Aug 2023 15:02:26 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 9:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9699/ DRY_RUN=false


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 9
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 Sep 2023 17:45:34 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/20377/9/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:

http://gerrit.cloudera.org:8080/#/c/20377/9/tests/query_test/test_observability.py@615
PS9, Line 615:       expected_strs = ["HostCpuIoWaitPercentage (50.000ms):",
This test failed in the Jenkins run.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 9
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 Sep 2023 22:09:15 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/13826/ : 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/20377
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 3
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Aug 2023 19:20:12 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 6:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/20377/5//COMMIT_MSG@15
PS5, Line 15: -Reduced profile metric samping interval to 50ms to support short-running
            :  queries.
> Done
Done


http://gerrit.cloudera.org:8080/#/c/20377/5/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:

http://gerrit.cloudera.org:8080/#/c/20377/5/be/src/util/runtime-profile.h@156
PS5, Line 156: bool is_system_;
> I actually missed some counters that were instantiated through macros. So K
nit: I still think that this can be removed because RuntimeProfile::Counter is separate from RuntimeProfile::TimeSeriesCounter. TotalBytesDequeued and TotalBytesReceived in KRPC code are TimeSeriesCounter and not Counter.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 6
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Aug 2023 18:32:19 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Hello Riza Suminto, David Rorke, Surya Hebbar, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................

IMPALA-12385: Enable Periodic metrics by default

This patch enables periodic metrics in query profiles by default and
changes the metric collectors to be more suitable for mixed workloads.

-Change default of resource_trace_ratio to 1
-Use samplng counters which can automatically resize for long queries
-Reduce metric interval to 50ms to support short-running queries
-Fragment metrics use the same sample interval as periodic metrics

Testing:
  Updated runtime-profile-test and test_observability.py for new defaults
  Manual inspection of query profile metrics for long-running queries

Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
---
M be/src/runtime/query-state.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/streaming-sampler.h
M common/thrift/Query.thrift
M tests/query_test/test_observability.py
8 files changed, 35 insertions(+), 31 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 2
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 2:

(5 comments)

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

http://gerrit.cloudera.org:8080/#/c/20377/1//COMMIT_MSG@12
PS1, Line 12: resource_trace_ratio to 1
AFAIK, there is a pretty significant overhead on always sampling this metrics. Seems like parsing /proc/stat, /proc/net/dev, /proc/diskstats does not come cheap. I'm not sure if this should be enabled by default.


http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/runtime/query-state.cc@221
PS1, Line 221: AddSamplingTimeSeriesCounter
Will this cause interpretation problem if different host happen to resize its sampling period differently?
In contrast, ChunkedTimeSeriesCounter does not resize it sampling period, right?


http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/util/periodic-counter-updater.cc
File be/src/util/periodic-counter-updater.cc:

http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/util/periodic-counter-updater.cc@30
PS1, Line 30: periodic_counter_update_period_ms, 50
I'm a bit concern about lowering this to 10x. Can the code in PeriodicCounterUpdater::UpdateLoop() keep up in such short sampling period under heavy-concurrent queries? It looks like PeriodicCounterUpdater is a singleton per impalad.


http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:

http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/util/runtime-profile-counters.h@807
PS1, Line 807: typedef StreamingSampler<int64_t, 64> StreamingCounterSampler;
If initial_period = 50ms, and MAX_SAMPLES = 64, that means it will take 3200ms before the sampling period doubled to 100ms. Will this hurt performance of short latency queries?


http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/util/streaming-sampler.h
File be/src/util/streaming-sampler.h:

http://gerrit.cloudera.org:8080/#/c/20377/1/be/src/util/streaming-sampler.h@40
PS1, Line 40: int initial_period
I'd rather keep this default to 500, but then add new parameter in AddSamplingTimeSeriesCounter for customized initial_period. I see this kind of counter is being used in other places like following:

be/src/runtime/fragment-instance-state.cc:  mem_usage_sampled_counter_ = profile()->AddSamplingTimeSeriesCounter("MemoryUsage",
be/src/runtime/fragment-instance-state.cc:  thread_usage_sampled_counter_ = profile()->AddSamplingTimeSeriesCounter("ThreadUsage",
be/src/runtime/krpc-data-stream-recvr.cc:      enqueue_profile_->AddSamplingTimeSeriesCounter("DeferredQueueSize", TUnit::UNIT,

Their sampling period should probably stay at 500, while sampling counters from host_profile_ starts at lower initial_period.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 2
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Aug 2023 16:23:59 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 7:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/13965/ : 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/20377
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 7
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Sat, 09 Sep 2023 12:35:56 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 8: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 8
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 Sep 2023 17:45:17 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Hello Riza Suminto, David Rorke, Surya Hebbar, Csaba Ringhofer, Michael Smith, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................

IMPALA-12385: Enable Periodic metrics by default

This patch enables periodic metrics in query profiles by default and
changes the metric collectors to be more suitable for mixed workloads.

-Changed default of resource_trace_ratio to 1.
-Changed profile metrics to use sampling counters which can automatically
 resize for long queries.
-Reduced profile metric samping interval to 50ms to support short-running
 queries.
-Changed fragment metrics to use the same sampling interval as periodic
 metrics.
-Added singleton PeriodicCounterUpdater and thread for updating system
 (KRPC) metrics at a different period than fragment metrics.
-Added new flag periodic_system_counter_update_period_ms for configuring
 system metric update period. Default is 500ms.

Example profile output:
- HostCpuIoWaitPercentage (400.000ms): 1, 0, 2, 3, 4, 6, 5, 2, 1, ...
- HostCpuSysPercentage (400.000ms): 1, 12, 10, 11, 11, 5, 3, 3, 3, ...
- HostCpuUserPercentage (400.000ms): 8, 46, 39, 39, 39, 29, 22, 23, ...

Testing:
-Updated runtime-profile-test and test_observability.py for new defaults
-Manual inspection of query profile metrics for long-running queries
-Tested for performance regression using 50 concurrent TPCH Q1 and with
 periodic_counter_update_period_ms=1 to verify that the periodic update
 loop does not affect peformance or become a bottleneck

Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
---
M be/src/runtime/exec-env.cc
M be/src/runtime/krpc-data-stream-recvr.cc
M be/src/runtime/query-state.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M be/src/util/streaming-sampler.h
M common/thrift/Query.thrift
M tests/query_test/test_observability.py
12 files changed, 147 insertions(+), 81 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/20377/10
-- 
To view, visit http://gerrit.cloudera.org:8080/20377
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 10
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>

[Impala-ASF-CR] IMPALA-12385: Enable Periodic metrics by default

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

Change subject: IMPALA-12385: Enable Periodic metrics by default
......................................................................


Patch Set 4:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/13881/ : 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/20377
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8e5cbfd4b324081158574ceb8f4b3a062a69fd1
Gerrit-Change-Number: 20377
Gerrit-PatchSet: 4
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Wed, 30 Aug 2023 04:20:55 +0000
Gerrit-HasComments: No