You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Quanlong Huang (Code Review)" <ge...@cloudera.org> on 2024/02/06 04:00:51 UTC

[Impala-ASF-CR] IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

Quanlong Huang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21001


Change subject: IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared
......................................................................

IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

TimeSeriesCounters are scheduled in PeriodicCounterUpdater at the time
when they are added. TimeSeriesCounterToJsonTest stops and clears them
to avoid the update thread interfere with the rest of the test. However,
the samples in SamplingTimeSeriesCounter are not cleared since it
doesn't implement the Clear() method of TimeSeriesCounter. This leads to
non-determined results depend on when the counter is stopped.

This patch adds the Clear() method for SamplingTimeSeriesCounter to
clear the samples. A Reset() method in StreamingSampler is added for
this purpose.

Test:
 - Ran TimeSeriesCounterToJsonTest 1500 times.

Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
---
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
4 files changed, 19 insertions(+), 2 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Gerrit-Change-Number: 21001
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>

[Impala-ASF-CR] IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

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

Change subject: IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared
......................................................................


Patch Set 5: Code-Review+1

ps5 looks OK to me. Let my try dry run this.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Gerrit-Change-Number: 21001
Gerrit-PatchSet: 5
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Feb 2024 05:49:22 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

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

Change subject: IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared
......................................................................


Patch Set 3:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Gerrit-Change-Number: 21001
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Wed, 07 Feb 2024 23:44:47 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

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

Change subject: IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared
......................................................................


Patch Set 2:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Gerrit-Change-Number: 21001
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Wed, 07 Feb 2024 23:40:39 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

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

Change subject: IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared
......................................................................


Patch Set 5: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Gerrit-Change-Number: 21001
Gerrit-PatchSet: 5
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Feb 2024 10:26:22 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

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

Change subject: IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared
......................................................................


Patch Set 1:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Gerrit-Change-Number: 21001
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Feb 2024 08:02:29 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

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

Change subject: IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared
......................................................................


Patch Set 1:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Gerrit-Change-Number: 21001
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Feb 2024 04:26:59 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

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

Change subject: IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared
......................................................................


Patch Set 1: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Gerrit-Change-Number: 21001
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Feb 2024 12:35:01 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

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

Change subject: IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared
......................................................................


Patch Set 4: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Gerrit-Change-Number: 21001
Gerrit-PatchSet: 4
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Thu, 08 Feb 2024 23:21:57 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Hello Laszlo Gaal, Kurt Deschler, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared
......................................................................

IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

TimeSeriesCounters are scheduled in PeriodicCounterUpdater at the time
when they are added. TimeSeriesCounterToJsonTest stops and clears them
to avoid the update thread interfere with the rest of the test. However,
the samples in SamplingTimeSeriesCounter are not cleared since it
doesn't implement the Clear() method of TimeSeriesCounter. This leads to
non-determined results depend on when the counter is stopped.

This patch adds the Clear() method for SamplingTimeSeriesCounter to
clear the samples. A Reset() method in StreamingSampler is added for
this purpose.

Test:
 - Ran TimeSeriesCounterToJsonTest 1500 times.

Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
---
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
4 files changed, 21 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Gerrit-Change-Number: 21001
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>

[Impala-ASF-CR] IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

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

Change subject: IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared
......................................................................


Patch Set 2:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/21001/1/be/src/util/streaming-sampler.h@49
PS1, Line 49: Reset
> resetting initial_period_ is missing here; I wonder if it's an omission,
'initial_period_' keeps the parameter value we got from the constructor. It's used to reset 'period_'. So after Reset(), everything comes back to the state of just constructing this instance.


http://gerrit.cloudera.org:8080/#/c/21001/1/be/src/util/streaming-sampler.h@105
PS1, Line 105: period_;
> This member seems to be unused (besides being initialized in the constructo
It's used to reset 'period_'. I'll add a comment.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Gerrit-Change-Number: 21001
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Wed, 07 Feb 2024 23:13:53 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

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

Change subject: IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared
......................................................................


Patch Set 5:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Gerrit-Change-Number: 21001
Gerrit-PatchSet: 5
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Fri, 09 Feb 2024 12:45:45 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

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

Change subject: IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared
......................................................................


Patch Set 5: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Gerrit-Change-Number: 21001
Gerrit-PatchSet: 5
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Feb 2024 13:57:52 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Hello Laszlo Gaal, Kurt Deschler, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared
......................................................................

IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

TimeSeriesCounters are scheduled in PeriodicCounterUpdater at the time
when they are added. TimeSeriesCounterToJsonTest stops and clears them
to avoid the update thread interfere with the rest of the test. However,
the samples in SamplingTimeSeriesCounter are not cleared since it
doesn't implement the Clear() method of TimeSeriesCounter. This leads to
non-determined results depend on when the counter is stopped.

This patch adds the Clear() method for SamplingTimeSeriesCounter to
clear the samples. A Reset() method in StreamingSampler is added for
this purpose.

Test:
 - Ran TimeSeriesCounterToJsonTest 1500 times.

Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
---
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
4 files changed, 22 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Gerrit-Change-Number: 21001
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>

[Impala-ASF-CR] IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

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

Change subject: IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared
......................................................................


Patch Set 4:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Gerrit-Change-Number: 21001
Gerrit-PatchSet: 4
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Thu, 08 Feb 2024 23:21:58 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

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

Change subject: IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared
......................................................................


Patch Set 5:

Updated the patch to only impact the test.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Gerrit-Change-Number: 21001
Gerrit-PatchSet: 5
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Fri, 09 Feb 2024 12:23:24 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

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

Change subject: IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared
......................................................................


Patch Set 1:

(2 comments)

Thanks for tackling this, Quanlong!
Looks good, except for a seemingly unused/unneeded member variable called out in the details.

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

http://gerrit.cloudera.org:8080/#/c/21001/1/be/src/util/streaming-sampler.h@49
PS1, Line 49: Reset
resetting initial_period_ is missing here; I wonder if it's an omission,
or initial_period_ is really unnecessary after all.
(just calling out the discrepancy here).


http://gerrit.cloudera.org:8080/#/c/21001/1/be/src/util/streaming-sampler.h@105
PS1, Line 105: initial_period_
This member seems to be unused (besides being initialized in the constructor).



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Gerrit-Change-Number: 21001
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Wed, 07 Feb 2024 17:23:21 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

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

Change subject: IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared
......................................................................


Patch Set 3: Code-Review+2

(2 comments)

I stand corrected; thanks for the explanation, Quanlong.
I shouldn't be doing code reviews while skiing I guess :)

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

http://gerrit.cloudera.org:8080/#/c/21001/1/be/src/util/streaming-sampler.h@49
PS1, Line 49: esets
> 'initial_period_' keeps the parameter value we got from the constructor. It
Done


http://gerrit.cloudera.org:8080/#/c/21001/1/be/src/util/streaming-sampler.h@105
PS1, Line 105: Storage period 
> It's used to reset 'period_'. I'll add a comment.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Gerrit-Change-Number: 21001
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Thu, 08 Feb 2024 14:14:02 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

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

Change subject: IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared
......................................................................


Patch Set 4:

(1 comment)

Thank Laszlo for the quick review! Enjoy the vocation!

Aborted the GVO job since I need to double check the impact on non-test codes.

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

http://gerrit.cloudera.org:8080/#/c/21001/4/be/src/runtime/query-state.cc@568
PS4, Line 568: host_profile_->ClearChunkedTime
This will now invoke SamplingTimeSeriesCounter::Clear(). It seems we just want to clear the ChunkedTimeSeriesCounters but keep the SamplingTimeSeriesCounters continues to do their work.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Gerrit-Change-Number: 21001
Gerrit-PatchSet: 4
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Fri, 09 Feb 2024 00:10:22 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Hello Laszlo Gaal, Kurt Deschler, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared
......................................................................

IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

TimeSeriesCounters are scheduled in PeriodicCounterUpdater at the time
when they are added. TimeSeriesCounterToJsonTest stops and clears them
to avoid the update thread interfere with the rest of the test. However,
the samples in SamplingTimeSeriesCounter are not cleared since it
doesn't implement the Clear() method of TimeSeriesCounter. This leads to
non-determined results depend on when the counter is stopped.

This patch adds a Reset() method for SamplingTimeSeriesCounter to clear
the samples. It's only used in the test. A Reset() method in
StreamingSampler is also added for this purpose.

Test:
 - Ran TimeSeriesCounterToJsonTest 100 times. It used to fail in 10 runs.

Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
---
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
4 files changed, 30 insertions(+), 4 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Gerrit-Change-Number: 21001
Gerrit-PatchSet: 5
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>

[Impala-ASF-CR] IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

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

Change subject: IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared
......................................................................


Patch Set 5:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Gerrit-Change-Number: 21001
Gerrit-PatchSet: 5
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Feb 2024 05:50:31 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

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

Change subject: IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared
......................................................................

IMPALA-11568: Fix SamplingTimeSeriesCounter not correctly cleared

TimeSeriesCounters are scheduled in PeriodicCounterUpdater at the time
when they are added. TimeSeriesCounterToJsonTest stops and clears them
to avoid the update thread interfere with the rest of the test. However,
the samples in SamplingTimeSeriesCounter are not cleared since it
doesn't implement the Clear() method of TimeSeriesCounter. This leads to
non-determined results depend on when the counter is stopped.

This patch adds a Reset() method for SamplingTimeSeriesCounter to clear
the samples. It's only used in the test. A Reset() method in
StreamingSampler is also added for this purpose.

Test:
 - Ran TimeSeriesCounterToJsonTest 100 times. It used to fail in 10 runs.

Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Reviewed-on: http://gerrit.cloudera.org:8080/21001
Reviewed-by: Riza Suminto <ri...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Wenzhe Zhou <wz...@cloudera.com>
---
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
4 files changed, 30 insertions(+), 4 deletions(-)

Approvals:
  Riza Suminto: Looks good to me, but someone else must approve
  Impala Public Jenkins: Verified
  Wenzhe Zhou: Looks good to me, approved

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I8bc2c94ad96899522b288708fcaf7413d4b99fa1
Gerrit-Change-Number: 21001
Gerrit-PatchSet: 6
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>