[IMPALA-6309] Clean up usage of Status(TErrorCode::type error, const ArgType& arg0, ...)

• Type: Task
• Status: Open
• Priority: Minor
• Resolution: Unresolved
• Affects Version/s: Impala 2.11.0
• Component/s: Backend
• Labels: cleanup
• Target Version: Product Backlog
• Epic Color: ghx-label-5
• Assignee: Unassigned
• Reporter: Michael Ho
• Created: 12/Dec/17 07:57
• Updated: 12/Dec/17 07:57

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.