You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Joe McDonnell (Code Review)" <ge...@cloudera.org> on 2020/12/10 22:44:01 UTC
[Impala-ASF-CR] IMPALA-10391: Fix LIRS edge case for single unprotected entry
Joe McDonnell has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16857
Change subject: IMPALA-10391: Fix LIRS edge case for single unprotected entry
......................................................................
IMPALA-10391: Fix LIRS edge case for single unprotected entry
When an unprotected entry is not in the recency list, a
lookup will cause it to be moved to be the newest
entry in the unprotected list. The fix for IMPALA-10127
introduced a regression when this happens when there is
exactly on entry in the unprotected list.
The code currently calls RemoveFromUnprotectedList()
followed by AddToUnprotectedList(). This now fails
because it is doing these operations without manipulating
the num_unprotected_ count. RemoveFromUnprotectedList()
clears out unprotected_list_front_, because num_unprotected_
is 1. However, AddToUnprotectedList() does not set it
back, because it only does that if num_unprotected_ is 0,
and the count is not changing.
This skips the remove/add in this case if there is exactly
one unprotected entry in the list.
Testing:
- Added a backend test for this specific case and verfied
that it failed before the fix and passes now
Change-Id: I8d21b619811a1a7baab1a92790f2ffc03e949131
---
M be/src/util/cache/lirs-cache-test.cc
M be/src/util/cache/lirs-cache.cc
2 files changed, 43 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/16857/1
--
To view, visit http://gerrit.cloudera.org:8080/16857
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8d21b619811a1a7baab1a92790f2ffc03e949131
Gerrit-Change-Number: 16857
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
[Impala-ASF-CR] IMPALA-10391: Fix LIRS edge case for single unprotected entry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16857 )
Change subject: IMPALA-10391: Fix LIRS edge case for single unprotected entry
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/16857
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8d21b619811a1a7baab1a92790f2ffc03e949131
Gerrit-Change-Number: 16857
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Dec 2020 03:23:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10391: Fix LIRS edge case for single unprotected entry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16857 )
Change subject: IMPALA-10391: Fix LIRS edge case for single unprotected entry
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6774/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/16857
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8d21b619811a1a7baab1a92790f2ffc03e949131
Gerrit-Change-Number: 16857
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 14 Dec 2020 21:10:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10391: Fix LIRS edge case for single unprotected entry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16857 )
Change subject: IMPALA-10391: Fix LIRS edge case for single unprotected entry
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6783/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/16857
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8d21b619811a1a7baab1a92790f2ffc03e949131
Gerrit-Change-Number: 16857
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Dec 2020 21:42:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10391: Fix LIRS edge case for single unprotected entry
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/16857 )
Change subject: IMPALA-10391: Fix LIRS edge case for single unprotected entry
......................................................................
Patch Set 2: Code-Review+2
Carry +2
--
To view, visit http://gerrit.cloudera.org:8080/16857
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8d21b619811a1a7baab1a92790f2ffc03e949131
Gerrit-Change-Number: 16857
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 14 Dec 2020 21:06:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10391: Fix LIRS edge case for single unprotected entry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16857 )
Change subject: IMPALA-10391: Fix LIRS edge case for single unprotected entry
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/7854/ : 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/16857
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8d21b619811a1a7baab1a92790f2ffc03e949131
Gerrit-Change-Number: 16857
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 14 Dec 2020 21:23:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10391: Fix LIRS edge case for single unprotected entry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16857 )
Change subject: IMPALA-10391: Fix LIRS edge case for single unprotected entry
......................................................................
Patch Set 3: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/6774/
--
To view, visit http://gerrit.cloudera.org:8080/16857
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8d21b619811a1a7baab1a92790f2ffc03e949131
Gerrit-Change-Number: 16857
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Dec 2020 00:20:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10391: Fix LIRS edge case for single unprotected entry
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16857 )
Change subject: IMPALA-10391: Fix LIRS edge case for single unprotected entry
......................................................................
IMPALA-10391: Fix LIRS edge case for single unprotected entry
When an unprotected entry is not in the recency list, a
lookup will cause it to be moved to be the newest
entry in the unprotected list. The fix for IMPALA-10127
introduced a regression when this happens when there is
exactly on entry in the unprotected list.
The code currently calls RemoveFromUnprotectedList()
followed by AddToUnprotectedList(). This now fails
because it is doing these operations without manipulating
the num_unprotected_ count. RemoveFromUnprotectedList()
clears out unprotected_list_front_, because num_unprotected_
is 1. However, AddToUnprotectedList() does not set it
back, because it only does that if num_unprotected_ is 0,
and the count is not changing.
This skips the remove/add in this case if there is exactly
one unprotected entry in the list.
Testing:
- Added a backend test for this specific case and verfied
that it failed before the fix and passes now
Change-Id: I8d21b619811a1a7baab1a92790f2ffc03e949131
Reviewed-on: http://gerrit.cloudera.org:8080/16857
Reviewed-by: Joe McDonnell <jo...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/util/cache/lirs-cache-test.cc
M be/src/util/cache/lirs-cache.cc
2 files changed, 44 insertions(+), 2 deletions(-)
Approvals:
Joe McDonnell: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/16857
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I8d21b619811a1a7baab1a92790f2ffc03e949131
Gerrit-Change-Number: 16857
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-10391: Fix LIRS edge case for single unprotected entry
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/16857 )
Change subject: IMPALA-10391: Fix LIRS edge case for single unprotected entry
......................................................................
Patch Set 3: Code-Review+2
Rebased, carrying +2
--
To view, visit http://gerrit.cloudera.org:8080/16857
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8d21b619811a1a7baab1a92790f2ffc03e949131
Gerrit-Change-Number: 16857
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 14 Dec 2020 21:09:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10391: Fix LIRS edge case for single unprotected entry
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16857 )
Change subject: IMPALA-10391: Fix LIRS edge case for single unprotected entry
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/16857
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8d21b619811a1a7baab1a92790f2ffc03e949131
Gerrit-Change-Number: 16857
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 14 Dec 2020 20:22:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10391: Fix LIRS edge case for single unprotected entry
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/16857 )
Change subject: IMPALA-10391: Fix LIRS edge case for single unprotected entry
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16857/1/be/src/util/cache/lirs-cache.cc
File be/src/util/cache/lirs-cache.cc:
http://gerrit.cloudera.org:8080/#/c/16857/1/be/src/util/cache/lirs-cache.cc@916
PS1, Line 916: if (num_unprotected_ != 1) {
> I guess it's impossible that num_unprotected_ is 0? Maybe add a DCHECK to a
Yes, num_unprotected_ must be > 0 in this case. Added a DCHECK here.
--
To view, visit http://gerrit.cloudera.org:8080/16857
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8d21b619811a1a7baab1a92790f2ffc03e949131
Gerrit-Change-Number: 16857
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 14 Dec 2020 21:02:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10391: Fix LIRS edge case for single unprotected entry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16857 )
Change subject: IMPALA-10391: Fix LIRS edge case for single unprotected entry
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/7834/ : 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/16857
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8d21b619811a1a7baab1a92790f2ffc03e949131
Gerrit-Change-Number: 16857
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 10 Dec 2020 23:06:02 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10391: Fix LIRS edge case for single unprotected entry
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16857
to look at the new patch set (#2).
Change subject: IMPALA-10391: Fix LIRS edge case for single unprotected entry
......................................................................
IMPALA-10391: Fix LIRS edge case for single unprotected entry
When an unprotected entry is not in the recency list, a
lookup will cause it to be moved to be the newest
entry in the unprotected list. The fix for IMPALA-10127
introduced a regression when this happens when there is
exactly on entry in the unprotected list.
The code currently calls RemoveFromUnprotectedList()
followed by AddToUnprotectedList(). This now fails
because it is doing these operations without manipulating
the num_unprotected_ count. RemoveFromUnprotectedList()
clears out unprotected_list_front_, because num_unprotected_
is 1. However, AddToUnprotectedList() does not set it
back, because it only does that if num_unprotected_ is 0,
and the count is not changing.
This skips the remove/add in this case if there is exactly
one unprotected entry in the list.
Testing:
- Added a backend test for this specific case and verfied
that it failed before the fix and passes now
Change-Id: I8d21b619811a1a7baab1a92790f2ffc03e949131
---
M be/src/util/cache/lirs-cache-test.cc
M be/src/util/cache/lirs-cache.cc
2 files changed, 44 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/16857/2
--
To view, visit http://gerrit.cloudera.org:8080/16857
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8d21b619811a1a7baab1a92790f2ffc03e949131
Gerrit-Change-Number: 16857
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-10391: Fix LIRS edge case for single unprotected entry
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16857 )
Change subject: IMPALA-10391: Fix LIRS edge case for single unprotected entry
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16857/1/be/src/util/cache/lirs-cache.cc
File be/src/util/cache/lirs-cache.cc:
http://gerrit.cloudera.org:8080/#/c/16857/1/be/src/util/cache/lirs-cache.cc@916
PS1, Line 916: if (num_unprotected_ != 1) {
I guess it's impossible that num_unprotected_ is 0? Maybe add a DCHECK to assert that it's >= 1.
--
To view, visit http://gerrit.cloudera.org:8080/16857
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8d21b619811a1a7baab1a92790f2ffc03e949131
Gerrit-Change-Number: 16857
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 14 Dec 2020 19:12:20 +0000
Gerrit-HasComments: Yes