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

Clean up usage of Status(TErrorCode::type error, const ArgType& arg0, ...)

    XMLWordPrintableJSON

    Details

    • Type: Task
    • Status: Open
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: Impala 2.11.0
    • Fix Version/s: None
    • Component/s: Backend
    • Labels:

      Description

      As shown in IMPALA-6308, the following idiom of not using hard coded TErrorCode::type with Status(TErrorCode::type error, const ArgType& arg0, ...) is pretty error prone as the format string changes. We should consider replacing error_code with constant TErrorCode::type like most other places in the code.

      void HdfsAvroScanner::SetStatusCorruptData(TErrorCode::type error_code) {
        DCHECK(parse_status_.ok());
        if (TestInfo::is_test()) {
          parse_status_ = Status(error_code, "test file", 123);
        } else {
          parse_status_ = Status(error_code, stream_->filename(), stream_->file_offset());
        }
      }
      
      void HdfsAvroScanner::SetStatusInvalidValue(TErrorCode::type error_code, int64_t len) {
        DCHECK(parse_status_.ok());
        if (TestInfo::is_test()) {
          parse_status_ = Status(error_code, "test file", len, 123);
        } else {
          parse_status_ = Status(error_code, stream_->filename(), len, stream_->file_offset());
        }
      }
      
      void HdfsAvroScanner::SetStatusValueOverflow(TErrorCode::type error_code, int64_t len,
          int64_t limit) {
        DCHECK(parse_status_.ok());
        if (TestInfo::is_test()) {
          parse_status_ = Status(error_code, "test file", len, limit, 123);
        } else {
          parse_status_ = Status(error_code, stream_->filename(), len, limit, stream_->file_offset());
        }
      }
      

      In general, Status(TErrorCode::type error, const ArgType& arg0,..) relies on the caller to pass the right number of arguments to match the number of substitution argument. In theory, if our test coverage is comprehensive, we should catch cases in which there is any mismatch. However, it's unclear if all usages of Status(TErrorCode::type error, const ArgType& arg0,..) are exercised. While the code coverage is the bigger issue here, it'd be nice to implement some compilation check to catch any mismatch between the number of arguments passed to Status() and the number of substitution arguments in the format string. This may be a follow on change after the above clean up is done.

        Attachments

          Activity

            People

            • Assignee:
              Unassigned
              Reporter:
              kwho Michael Ho
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated: