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

HdfsScanners drops Status on the floor in multiple places

    Details

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

      Description

      I was playing around with _attribute_((warn_unused_result)) (IMPALA-2615) and found this:

        void AttachPool(MemPool* pool, bool commit_batch) {
          DCHECK(scan_node_->HasRowBatchQueue());
          DCHECK(batch_ != NULL);
          DCHECK(pool != NULL);
          batch_->tuple_data_pool()->AcquireData(pool, false);
          if (commit_batch) CommitRows(0); <== drops Status
        }
      

      This may have been valid at some point in time, but can now leave the scanner in an unexpected state if allocating the new RowBatch fails.

      BaseSequenceScanner::ProcessSplit() also drops the AddScanRanges() status.

        Activity

        Hide
        tarmstrong Tim Armstrong added a comment -

        This also affects Impala-lzo so there should be a corresponding change made there.

        Show
        tarmstrong Tim Armstrong added a comment - This also affects Impala-lzo so there should be a corresponding change made there.
        Hide
        tarmstrong Tim Armstrong added a comment -

        IMPALA-4391: fix dropped statuses in scanners

        As far as I'm aware we haven't seen any failures related to these in
        practice, but fixing them as a preventative measure.

        Testing:
        I was unable to reproduce this easily by adding a failpoint. I suspect
        that stress testing of the affected file formats would have eventually
        found this, because of the similarity to IMPALA-3962.

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

        Show
        tarmstrong Tim Armstrong added a comment - IMPALA-4391 : fix dropped statuses in scanners As far as I'm aware we haven't seen any failures related to these in practice, but fixing them as a preventative measure. Testing: I was unable to reproduce this easily by adding a failpoint. I suspect that stress testing of the affected file formats would have eventually found this, because of the similarity to IMPALA-3962 . Change-Id: I7c47f8b29a20dc74ad9e9e1c58b3d7ed95de4d35 Reviewed-on: http://gerrit.cloudera.org:8080/4938 Reviewed-by: Tim Armstrong <tarmstrong@cloudera.com> Tested-by: Internal Jenkins
        Hide
        tarmstrong Tim Armstrong added a comment -

        The previous commit fixed all the cases in Apache Impala

        Show
        tarmstrong Tim Armstrong added a comment - The previous commit fixed all the cases in Apache Impala
        Hide
        tarmstrong Tim Armstrong added a comment -

        For reference, a related commit made it into Impala-lzo

        IMPALA-4391: fix dropped status in scanners

        Handle non-ok statuses from AttachPool() that were added in the
        corresponding Impala commit. Also use the utility code from Impala
        to check whether abort_on_error applies.

        Also remove the old status api code - there have been many other
        breaking API changes in the meantime so there is no point maintaining
        it.

        Change-Id: I67e45fc05596ff1b32a50d4ea3c917ba0cc63ab8

        Show
        tarmstrong Tim Armstrong added a comment - For reference, a related commit made it into Impala-lzo IMPALA-4391 : fix dropped status in scanners Handle non-ok statuses from AttachPool() that were added in the corresponding Impala commit. Also use the utility code from Impala to check whether abort_on_error applies. Also remove the old status api code - there have been many other breaking API changes in the meantime so there is no point maintaining it. Change-Id: I67e45fc05596ff1b32a50d4ea3c917ba0cc63ab8

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development