Uploaded image for project: 'IMPALA'
  1. IMPALA
  2. IMPALA-4104

Fix TestEnv and reenable DCHECK in MemTracker::ConsumeLocal()

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: Impala 2.8.0
    • Fix Version/s: Impala 2.8.0
    • Component/s: Backend
    • Labels:
      None

      Description

      Adding the bottom DCHECK:

        void ConsumeLocal(int64_t bytes, MemTracker* end_tracker) {
          DCHECK(consumption_metric_ == NULL) << "Should not be called on root.";
          for (int i = 0; i < all_trackers_.size(); ++i) {
            if (all_trackers_[i] == end_tracker) return;
            DCHECK(!all_trackers_[i]->has_limit());
            all_trackers_[i]->consumption_->Add(bytes);
          }
          DCHECK(false) << "end_tracker is not an ancestor";
        }
      

      caused backend test failures, e.g. some BufferedBlockMgr tests because they set up their MemTrackers in a strange way. The DCHECK is useful so this should be fixed.

        Activity

        Hide
        tarmstrong Tim Armstrong added a comment -

        IMPALA-4104: add DCHECK to ConsumeLocal() and fix tests

        The TestEnv used for the backend tests does not connect up the
        MemTracker hierarchy in the expected way. This caused the valid
        DCHECK in ConsumeLocal() to be triggered in backend tests.

        This change fixes TestEnv to set up MemTrackers with the normal
        hierarchy, as shown below, and fixes the tests to deal with the fallout
        of that.

        (Process)

        (Query)----------

         

        (Block Mgr) (Fragment instance)

        Change-Id: Iadcbe96a9f1bf19872436211b049cebf39b0afe7
        Reviewed-on: http://gerrit.cloudera.org:8080/4531
        Reviewed-by: Tim Armstrong <tarmstrong@cloudera.com>
        Tested-by: Internal Jenkins

        M be/src/exec/hash-table-test.cc
        M be/src/runtime/buffered-block-mgr-test.cc
        M be/src/runtime/buffered-block-mgr.h
        M be/src/runtime/buffered-tuple-stream-test.cc
        M be/src/runtime/mem-tracker.h
        M be/src/runtime/test-env.cc
        M be/src/runtime/test-env.h
        7 files changed, 135 insertions, 119 deletions

        Approvals:
        Internal Jenkins: Verified
        Tim Armstrong: Looks good to me, approved


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

        Show
        tarmstrong Tim Armstrong added a comment - IMPALA-4104 : add DCHECK to ConsumeLocal() and fix tests The TestEnv used for the backend tests does not connect up the MemTracker hierarchy in the expected way. This caused the valid DCHECK in ConsumeLocal() to be triggered in backend tests. This change fixes TestEnv to set up MemTrackers with the normal hierarchy, as shown below, and fixes the tests to deal with the fallout of that. (Process) (Query)----------   (Block Mgr) (Fragment instance) Change-Id: Iadcbe96a9f1bf19872436211b049cebf39b0afe7 Reviewed-on: http://gerrit.cloudera.org:8080/4531 Reviewed-by: Tim Armstrong <tarmstrong@cloudera.com> Tested-by: Internal Jenkins — M be/src/exec/hash-table-test.cc M be/src/runtime/buffered-block-mgr-test.cc M be/src/runtime/buffered-block-mgr.h M be/src/runtime/buffered-tuple-stream-test.cc M be/src/runtime/mem-tracker.h M be/src/runtime/test-env.cc M be/src/runtime/test-env.h 7 files changed, 135 insertions , 119 deletions Approvals: Internal Jenkins: Verified Tim Armstrong: Looks good to me, approved – To view, visit http://gerrit.cloudera.org:8080/4531 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

          People

          • Assignee:
            tarmstrong Tim Armstrong
            Reporter:
            tarmstrong Tim Armstrong
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development