You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Riza Suminto (Code Review)" <ge...@cloudera.org> on 2021/08/31 02:23:10 UTC
[Impala-ASF-CR] IMPALA-10883: Do not override existing counters with empty profile
Riza Suminto has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17819
Change subject: IMPALA-10883: Do not override existing counters with empty profile
......................................................................
IMPALA-10883: Do not override existing counters with empty profile
Some profile information was missing when gen_experimental_profile flag
is enabled. This is because profile aggregation in the coordinator does
not anticipate a partial update from a backend. From a backend
perspective, if a fragment instance has sent its final report, that
instance will not participate in the subsequent profile report.
Therefore, the aggregated counters that belong to the finished instance
will be empty. This patch adds empty checks in the aggregation of
input_profile_names_, TAggTimeSeriesCounter, and TAggEventSequence to
prevent the existing value from being overridden by an empty profile.
Testing:
- Add BE test CountersTest.PartialUpdate
Change-Id: I9bb179bf739ffaa4e5ec8dc911480ac835ae387f
---
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
2 files changed, 144 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/17819/1
--
To view, visit http://gerrit.cloudera.org:8080/17819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9bb179bf739ffaa4e5ec8dc911480ac835ae387f
Gerrit-Change-Number: 17819
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-10883: Do not override existing counters with empty profile
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17819 )
Change subject: IMPALA-10883: Do not override existing counters with empty profile
......................................................................
IMPALA-10883: Do not override existing counters with empty profile
Some profile information was missing when gen_experimental_profile flag
is enabled. This is because profile aggregation in the coordinator does
not anticipate a partial update from a backend. From a backend
perspective, if a fragment instance has sent its final report, that
instance will not participate in the subsequent profile report.
Therefore, the aggregated counters that belong to the finished instance
will be empty. This patch adds empty checks in the aggregation of
input_profile_names_, TAggTimeSeriesCounter, and TAggEventSequence to
prevent the existing value from being overridden by an empty profile.
Testing:
- Add BE test CountersTest.PartialUpdate
Change-Id: I9bb179bf739ffaa4e5ec8dc911480ac835ae387f
Reviewed-on: http://gerrit.cloudera.org:8080/17819
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
2 files changed, 139 insertions(+), 1 deletion(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/17819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9bb179bf739ffaa4e5ec8dc911480ac835ae387f
Gerrit-Change-Number: 17819
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-10883: Do not override existing counters with empty profile
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17819 )
Change subject: IMPALA-10883: Do not override existing counters with empty profile
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/17819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bb179bf739ffaa4e5ec8dc911480ac835ae387f
Gerrit-Change-Number: 17819
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Sep 2021 06:38:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10883: Do not override existing counters with empty profile
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/17819 )
Change subject: IMPALA-10883: Do not override existing counters with empty profile
......................................................................
Patch Set 2:
(2 comments)
Thanks Quanlong! I address your comments in patch set 2.
http://gerrit.cloudera.org:8080/#/c/17819/1/be/src/util/runtime-profile-test.cc
File be/src/util/runtime-profile-test.cc:
http://gerrit.cloudera.org:8080/#/c/17819/1/be/src/util/runtime-profile-test.cc@1489
PS1, Line 1489: (i - offset + 1) * 11
> This should be 10 when i == offset due to line 1566. Or should we set the c
Thanks for catching this! I fix line 1566 to set the counter value to 11.
http://gerrit.cloudera.org:8080/#/c/17819/1/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:
http://gerrit.cloudera.org:8080/#/c/17819/1/be/src/util/runtime-profile.cc@627
PS1, Line 627: int idx = start_idx + i;
> I traced the codes generating 'values' but still not sure whether a valid '
Done
--
To view, visit http://gerrit.cloudera.org:8080/17819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bb179bf739ffaa4e5ec8dc911480ac835ae387f
Gerrit-Change-Number: 17819
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Sep 2021 15:38:45 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10883: Do not override existing counters with empty profile
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17819 )
Change subject: IMPALA-10883: Do not override existing counters with empty profile
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7457/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/17819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bb179bf739ffaa4e5ec8dc911480ac835ae387f
Gerrit-Change-Number: 17819
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Sep 2021 00:31:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10883: Do not override existing counters with empty profile
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17819
to look at the new patch set (#2).
Change subject: IMPALA-10883: Do not override existing counters with empty profile
......................................................................
IMPALA-10883: Do not override existing counters with empty profile
Some profile information was missing when gen_experimental_profile flag
is enabled. This is because profile aggregation in the coordinator does
not anticipate a partial update from a backend. From a backend
perspective, if a fragment instance has sent its final report, that
instance will not participate in the subsequent profile report.
Therefore, the aggregated counters that belong to the finished instance
will be empty. This patch adds empty checks in the aggregation of
input_profile_names_, TAggTimeSeriesCounter, and TAggEventSequence to
prevent the existing value from being overridden by an empty profile.
Testing:
- Add BE test CountersTest.PartialUpdate
Change-Id: I9bb179bf739ffaa4e5ec8dc911480ac835ae387f
---
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
2 files changed, 139 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/17819/2
--
To view, visit http://gerrit.cloudera.org:8080/17819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9bb179bf739ffaa4e5ec8dc911480ac835ae387f
Gerrit-Change-Number: 17819
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-10883: Do not override existing counters with empty profile
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17819 )
Change subject: IMPALA-10883: Do not override existing counters with empty profile
......................................................................
Patch Set 2: Code-Review+2
LGTM. Thanks for fixing this!
--
To view, visit http://gerrit.cloudera.org:8080/17819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bb179bf739ffaa4e5ec8dc911480ac835ae387f
Gerrit-Change-Number: 17819
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Sep 2021 00:30:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10883: Do not override existing counters with empty profile
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17819 )
Change subject: IMPALA-10883: Do not override existing counters with empty profile
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9406/ : 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/17819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bb179bf739ffaa4e5ec8dc911480ac835ae387f
Gerrit-Change-Number: 17819
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Aug 2021 02:46:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10883: Do not override existing counters with empty profile
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17819 )
Change subject: IMPALA-10883: Do not override existing counters with empty profile
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/17819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bb179bf739ffaa4e5ec8dc911480ac835ae387f
Gerrit-Change-Number: 17819
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Sep 2021 00:31:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10883: Do not override existing counters with empty profile
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17819 )
Change subject: IMPALA-10883: Do not override existing counters with empty profile
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9427/ : 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/17819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bb179bf739ffaa4e5ec8dc911480ac835ae387f
Gerrit-Change-Number: 17819
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Sep 2021 15:55:55 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10883: Do not override existing counters with empty profile
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17819 )
Change subject: IMPALA-10883: Do not override existing counters with empty profile
......................................................................
Patch Set 1:
(2 comments)
Sorry to be late here due to my vacation. I'll try to unblock this in the following days. The fix makes sense to me!
http://gerrit.cloudera.org:8080/#/c/17819/1/be/src/util/runtime-profile-test.cc
File be/src/util/runtime-profile-test.cc:
http://gerrit.cloudera.org:8080/#/c/17819/1/be/src/util/runtime-profile-test.cc@1489
PS1, Line 1489: (i - offset + 1) * 11
This should be 10 when i == offset due to line 1566. Or should we set the counter value to 11 at line 1566?
http://gerrit.cloudera.org:8080/#/c/17819/1/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:
http://gerrit.cloudera.org:8080/#/c/17819/1/be/src/util/runtime-profile.cc@627
PS1, Line 627: if (LIKELY(!tcounter.values[i].empty())) {
I traced the codes generating 'values' but still not sure whether a valid 'values' can be empty, e.g. at the begining or in some corner cases of ChunkedTimeSeriesCounter. Indeed, what we want to avoid is invalid/empty values overwriting valid values. Can we adjust this to the following?
if (UNLIKELY(tcounter.values[i].empty() && !dst.values[idx].empty())) continue;
--
To view, visit http://gerrit.cloudera.org:8080/17819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bb179bf739ffaa4e5ec8dc911480ac835ae387f
Gerrit-Change-Number: 17819
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Sep 2021 13:52:30 +0000
Gerrit-HasComments: Yes