You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org> on 2020/01/16 10:40:07 UTC
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Zoltan Borok-Nagy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15050
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Before this fix MemTracker::Consume() and Release() could take
negative numbers as arguments. In that case Consume(-val) would invoke
Release(val) and vice versa.
On the other hand, TryConsume() was a no-op for negative values and
returned success. This could lead to errors in the following case:
TryConsume(-42); // no-op
...
Release(-42); // -> Consume(42);
And in the end the mem tracker would think there is 42 bytes of
unallocated memory.
This commit changes mem-tracker to forbid negative values.
Testing:
* ran exhaustive tests
Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
---
M be/src/runtime/mem-tracker-test.cc
M be/src/runtime/mem-tracker.h
2 files changed, 30 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/15050/1
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15050/5/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:
http://gerrit.cloudera.org:8080/#/c/15050/5/be/src/runtime/mem-tracker.h@175
PS5, Line 175: if (UNLIKELY(bytes == 0)) return true;
> Shouldn't it be handled like at Release? Since there is a DCHECK for this a
The above DCHECK allows the value 0 and it should return success. We need this branch to avoid updating 'consumption_' when the argument is zero, even when we fetch the consumption from a metric. This behavior is aligned with the behavior of Consume()/Release().
http://gerrit.cloudera.org:8080/#/c/15050/5/be/src/runtime/mem-tracker.h@218
PS5, Line 218: < 0
> nit: did you mean <= ?
< 0 hits DCHECK in debug, only needed for release.
== 0 is needed in both release and debug to avoid updating 'consumption_'
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Jan 2020 14:12:10 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5468/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 27 Jan 2020 12:45:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Norbert Luksa (Code Review)" <ge...@cloudera.org>.
Norbert Luksa has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15050/5/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:
http://gerrit.cloudera.org:8080/#/c/15050/5/be/src/runtime/mem-tracker.h@175
PS5, Line 175: if (UNLIKELY(bytes == 0)) return true;
Shouldn't it be handled like at Release? Since there is a DCHECK for this above, we should not even reach this line. You also say in the commit message that the consumption is not updated when the bytes are zero.
http://gerrit.cloudera.org:8080/#/c/15050/5/be/src/runtime/mem-tracker.h@218
PS5, Line 218: < 0
nit: did you mean <= ?
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Jan 2020 13:57:38 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15050/1/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:
http://gerrit.cloudera.org:8080/#/c/15050/1/be/src/runtime/mem-tracker.h@171
PS1, Line 171: WARN_UNUSED_RESULT
: bool TryConsume(int64_t bytes, MemLimit mode = MemLimit::HARD) {
: DCHECK_GE(bytes, 0);
: DCHECK(!closed_) << label_;
> Added checks for the bytes < 0 case and return false in TryConsume(). Unfor
nit: a comment like " // needed in RELEASE, hits DCHECK in DEBUG" would make the intention clearer (+ would still fit in 90 chars)
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jan 2020 14:08:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15050/1/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:
http://gerrit.cloudera.org:8080/#/c/15050/1/be/src/runtime/mem-tracker.h@171
PS1, Line 171: DCHECK_GE(bytes, 0);
: DCHECK(!closed_) << label_;
: if (consumption_metric_ != nullptr) RefreshConsumptionFromMetric();
: if (UNLIKELY(bytes == 0)) return true;
> I'd consider removing the bytes == 0 special case, since it's not common so
Added checks for the bytes < 0 case and return false in TryConsume(). Unfortunately Consume() and Release() don't have return values.
http://gerrit.cloudera.org:8080/#/c/15050/1/be/src/runtime/mem-tracker.h@215
PS1, Line 215: if (bytes == 0) return;
> Same here - consider removing the special case.
Removed the check against 0, but Based on Csaba's suggestion I changed it to check if bytes < 0.
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jan 2020 10:40:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15050/1/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:
http://gerrit.cloudera.org:8080/#/c/15050/1/be/src/runtime/mem-tracker.h@215
PS1, Line 215: if (bytes == 0) return;
> Removed the check against 0, but Based on Csaba's suggestion I changed it t
Based on the backend tests when Consume()/Release() is called with 0 as argument it shouldn't update 'consumption_' even when 'consumption_metric_' is not null.
TryConsume() also behaved differently in this regard so I also resolved that inconsistency.
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Jan 2020 13:50:31 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 1:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/5444/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Jan 2020 20:40:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 27 Jan 2020 11:53:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Jan 2020 17:03:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5493/ : 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/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Jan 2020 14:30:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5450/ : 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/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jan 2020 11:19:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5448/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Jan 2020 17:03:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15050/1/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:
http://gerrit.cloudera.org:8080/#/c/15050/1/be/src/runtime/mem-tracker.h@171
PS1, Line 171: DCHECK_GE(bytes, 0);
: DCHECK(!closed_) << label_;
: if (consumption_metric_ != nullptr) RefreshConsumptionFromMetric();
: if (UNLIKELY(bytes == 0)) return true;
> nit: a comment like " // needed in RELEASE, hits DCHECK in DEBUG" would mak
Done
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jan 2020 14:41:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Norbert Luksa (Code Review)" <ge...@cloudera.org>.
Norbert Luksa has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Jan 2020 15:17:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 4: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5448/
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Jan 2020 21:53:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Norbert Luksa, Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15050
to look at the new patch set (#5).
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Before this fix MemTracker::Consume() and Release() could take
negative numbers as arguments. In that case Consume(-val) would invoke
Release(val) and vice versa.
On the other hand, TryConsume() was a no-op for negative values and
returned success. This could lead to errors in the following case:
TryConsume(-42); // no-op
...
Release(-42); // -> Consume(42);
And in the end the mem tracker would think there is 42 bytes of
unallocated memory.
This commit changes mem-tracker to forbid negative values.
Another inconsistency was found between Consume()/Release() and
TryConsume(). When Consume()/Release() was invoked with zero as
argument they didn't update 'consumption_' when 'consumption_metric_'
was not null. On the other hand, TryConsume() being invoked with
zero did update 'consumption_' from the metric. I fixed this
inconsistency as well, i.e. now 'consumption_' is not updated
when the argument is zero.
Testing:
* ran exhaustive tests
Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
---
M be/src/runtime/mem-tracker-test.cc
M be/src/runtime/mem-tracker.h
2 files changed, 41 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/15050/5
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 1: Code-Review+2
(2 comments)
Thanks for cleaning this up, it helps to have stricter invariants in code like this. I had one suggestion but very happy with the code change as-is.
http://gerrit.cloudera.org:8080/#/c/15050/1/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:
http://gerrit.cloudera.org:8080/#/c/15050/1/be/src/runtime/mem-tracker.h@171
PS1, Line 171: DCHECK_GE(bytes, 0);
: DCHECK(!closed_) << label_;
: if (consumption_metric_ != nullptr) RefreshConsumptionFromMetric();
: if (UNLIKELY(bytes == 0)) return true;
> I agree with adding the DCHECK to catch such calls during tests, but I am c
I'd consider removing the bytes == 0 special case, since it's not common so not worth optimising and the code below should handle it fine.
(Not that important, OK to ignore)
http://gerrit.cloudera.org:8080/#/c/15050/1/be/src/runtime/mem-tracker.h@215
PS1, Line 215: if (bytes == 0) return;
Same here - consider removing the special case.
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Jan 2020 17:19:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15050/1/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:
http://gerrit.cloudera.org:8080/#/c/15050/1/be/src/runtime/mem-tracker.h@171
PS1, Line 171: DCHECK_GE(bytes, 0);
: DCHECK(!closed_) << label_;
: if (consumption_metric_ != nullptr) RefreshConsumptionFromMetric();
: if (UNLIKELY(bytes == 0)) return true;
I agree with adding the DCHECK to catch such calls during tests, but I am concerned about this issue occurring in a release build (e.g. if the value comes from a corrupt file). My preference is to return with false in the < 0 case, and add similar logic to other consume/release functions.
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Jan 2020 13:48:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Jan 2020 16:51:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 6: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 27 Jan 2020 17:33:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
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/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Before this fix MemTracker::Consume() and Release() could take
negative numbers as arguments. In that case Consume(-val) would invoke
Release(val) and vice versa.
On the other hand, TryConsume() was a no-op for negative values and
returned success. This could lead to errors in the following case:
TryConsume(-42); // no-op
...
Release(-42); // -> Consume(42);
And in the end the mem tracker would think there is 42 bytes of
unallocated memory.
This commit changes mem-tracker to forbid negative values.
Another inconsistency was found between Consume()/Release() and
TryConsume(). When Consume()/Release() was invoked with zero as
argument they didn't update 'consumption_' when 'consumption_metric_'
was not null. On the other hand, TryConsume() being invoked with
zero did update 'consumption_' from the metric. I fixed this
inconsistency as well, i.e. now 'consumption_' is not updated
when the argument is zero.
Testing:
* ran exhaustive tests
Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Reviewed-on: http://gerrit.cloudera.org:8080/15050
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/runtime/mem-tracker-test.cc
M be/src/runtime/mem-tracker.h
2 files changed, 41 insertions(+), 25 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 27 Jan 2020 12:45:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5454/ : 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/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jan 2020 15:27:39 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Norbert Luksa, Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15050
to look at the new patch set (#2).
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Before this fix MemTracker::Consume() and Release() could take
negative numbers as arguments. In that case Consume(-val) would invoke
Release(val) and vice versa.
On the other hand, TryConsume() was a no-op for negative values and
returned success. This could lead to errors in the following case:
TryConsume(-42); // no-op
...
Release(-42); // -> Consume(42);
And in the end the mem tracker would think there is 42 bytes of
unallocated memory.
This commit changes mem-tracker to forbid negative values.
Testing:
* ran exhaustive tests
Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
---
M be/src/runtime/mem-tracker-test.cc
M be/src/runtime/mem-tracker.h
2 files changed, 32 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/15050/2
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jan 2020 14:54:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Norbert Luksa, Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15050
to look at the new patch set (#3).
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Before this fix MemTracker::Consume() and Release() could take
negative numbers as arguments. In that case Consume(-val) would invoke
Release(val) and vice versa.
On the other hand, TryConsume() was a no-op for negative values and
returned success. This could lead to errors in the following case:
TryConsume(-42); // no-op
...
Release(-42); // -> Consume(42);
And in the end the mem tracker would think there is 42 bytes of
unallocated memory.
This commit changes mem-tracker to forbid negative values.
Testing:
* ran exhaustive tests
Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
---
M be/src/runtime/mem-tracker-test.cc
M be/src/runtime/mem-tracker.h
2 files changed, 32 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/15050/3
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Posted by "Norbert Luksa (Code Review)" <ge...@cloudera.org>.
Norbert Luksa has posted comments on this change. ( http://gerrit.cloudera.org:8080/15050 )
Change subject: IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15050/5/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:
http://gerrit.cloudera.org:8080/#/c/15050/5/be/src/runtime/mem-tracker.h@175
PS5, Line 175: if (UNLIKELY(bytes == 0)) return true;
> The above DCHECK allows the value 0 and it should return success. We need t
Right, for some reasons I thought it's a DCHECK_LE, apologies!
--
To view, visit http://gerrit.cloudera.org:8080/15050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Gerrit-Change-Number: 15050
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Norbert Luksa <no...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Jan 2020 14:20:36 +0000
Gerrit-HasComments: Yes