Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Enhancement](Log) Reduce usage of log fatal(PART I) #42344

Merged
merged 35 commits into from
Dec 19, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions be/src/common/dwarf.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -180,9 +180,9 @@ Dwarf::Dwarf(const std::shared_ptr<Elf>& elf)

Dwarf::Section::Section(std::string_view d) : is64_bit(false), data(d) {}

#define SAFE_CHECK(cond, ...) \
do { \
if (!(cond)) LOG(FATAL) << fmt::format(__VA_ARGS__); \
#define SAFE_CHECK(cond, ...) \
do { \
if (!(cond)) throw Exception(Status::FatalError(__VA_ARGS__)); \
} while (false)

namespace {
Expand Down
34 changes: 18 additions & 16 deletions be/src/common/elf.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
// https://github.com/ClickHouse/ClickHouse/blob/master/src/Common/Elf.cpp
// and modified by Doris

#include "common/exception.h"
#if defined(__ELF__) && !defined(__FreeBSD__)

#include <common/elf.h>
Expand All @@ -40,21 +41,22 @@ Elf::Elf(const std::string& path) {
std::error_code ec;
elf_size = std::filesystem::file_size(_file, ec);
if (ec) {
LOG(FATAL) << fmt::format("failed to get file size {}: ({}), {}", _file.native(),
ec.value(), ec.message());
throw Exception(Status::FatalError("failed to get file size {}: ({}), {}", _file.native(),
ec.value(), ec.message()));
}
/// Check if it's an elf.
if (elf_size < sizeof(ElfEhdr)) {
LOG(FATAL) << fmt::format("The size of supposedly ELF file '{}' is too small", path);
throw Exception(
Status::FatalError("The size of supposedly ELF file '{}' is too small", path));
}
RETRY_ON_EINTR(_fd, open(_file.c_str(), O_RDONLY));
if (_fd < 0) {
LOG(FATAL) << fmt::format("failed to open {}", _file.native());
throw Exception(Status::FatalError("failed to open {}", _file.native()));
}
mapped = static_cast<char*>(mmap(nullptr, elf_size, PROT_READ, MAP_SHARED, _fd, 0));
if (MAP_FAILED == mapped) {
LOG(FATAL) << fmt::format("MMappedFileDescriptor: Cannot mmap {}, read from {}.", elf_size,
path);
throw Exception(Status::FatalError("MMappedFileDescriptor: Cannot mmap {}, read from {}.",
elf_size, path));
}

header = reinterpret_cast<const ElfEhdr*>(mapped);
Expand All @@ -63,7 +65,7 @@ Elf::Elf(const std::string& path) {
"\x7F"
"ELF",
4) != 0) {
LOG(FATAL) << fmt::format("The file '{}' is not ELF according to magic", path);
throw Exception(Status::FatalError("The file '{}' is not ELF according to magic", path));
}

/// Get section header.
Expand All @@ -72,8 +74,8 @@ Elf::Elf(const std::string& path) {

if (!section_header_offset || !section_header_num_entries ||
section_header_offset + section_header_num_entries * sizeof(ElfShdr) > elf_size) {
LOG(FATAL) << fmt::format(
"The ELF '{}' is truncated (section header points after end of file)", path);
throw Exception(Status::FatalError(
"The ELF '{}' is truncated (section header points after end of file)", path));
}

section_headers = reinterpret_cast<const ElfShdr*>(mapped + section_header_offset);
Expand All @@ -84,15 +86,15 @@ Elf::Elf(const std::string& path) {
});

if (!section_names_strtab) {
LOG(FATAL) << fmt::format("The ELF '{}' doesn't have string table with section names",
path);
throw Exception(Status::FatalError(
"The ELF '{}' doesn't have string table with section names", path));
}

ElfOff section_names_offset = section_names_strtab->header.sh_offset;
if (section_names_offset >= elf_size) {
LOG(FATAL) << fmt::format(
throw Exception(Status::FatalError(
"The ELF '{}' is truncated (section names string table points after end of file)",
path);
path));
}
section_names = reinterpret_cast<const char*>(mapped + section_names_offset);

Expand All @@ -103,8 +105,8 @@ Elf::Elf(const std::string& path) {

if (!program_header_offset || !program_header_num_entries ||
program_header_offset + program_header_num_entries * sizeof(ElfPhdr) > elf_size) {
LOG(FATAL) << fmt::format(
"The ELF '{}' is truncated (program header points after end of file)", path);
throw Exception(Status::FatalError(
"The ELF '{}' is truncated (program header points after end of file)", path));
}
program_headers = reinterpret_cast<const ElfPhdr*>(mapped + program_header_offset);
}
Expand Down Expand Up @@ -219,7 +221,7 @@ std::string Elf::getStoredBinaryHash() const {

const char* Elf::Section::name() const {
if (!elf.section_names) {
LOG(FATAL) << fmt::format("Section names are not initialized");
throw Exception(Status::FatalError("Section names are not initialized"));
}

/// TODO buffer overflow is possible, we may need to check strlen.
Expand Down
3 changes: 2 additions & 1 deletion be/src/common/exception.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ Exception::Exception(int code, const std::string_view& msg) {
_err_msg->_stack = get_stack_trace();
}
if (config::exit_on_exception) {
LOG(FATAL) << "[ExitOnException] error code: " << code << ", message: " << msg;
throw Exception(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont change this. we want always exit on exception here.

Status::FatalError("[ExitOnException] error code: {}, message: {}", code, msg));
}
}
} // namespace doris
11 changes: 10 additions & 1 deletion be/src/common/status.h
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,8 @@ namespace ErrorCode {
E(ENTRY_NOT_FOUND, -7002, false); \
E(INVALID_TABLET_STATE, -7211, false); \
E(ROWSETS_EXPIRED, -7311, false); \
E(CGROUP_ERROR, -7411, false);
E(CGROUP_ERROR, -7411, false); \
E(FATAL_ERROR, -7412, false);

// Define constexpr int error_code_name = error_code_value
#define M(NAME, ERRORCODE, ENABLESTACKTRACE) constexpr int NAME = ERRORCODE;
Expand Down Expand Up @@ -445,6 +446,14 @@ class [[nodiscard]] Status {

static Status OK() { return {}; }

template <bool stacktrace = true, typename... Args>
static Status FatalError(std::string_view msg, Args&&... args) {
#ifndef NDEBUG
LOG(FATAL) << fmt::format(msg, std::forward<Args>(args)...);
#endif
return Error<ErrorCode::FATAL_ERROR, stacktrace>(msg, std::forward<Args>(args)...);
}

// default have stacktrace. could disable manually.
#define ERROR_CTOR(name, code) \
template <bool stacktrace = true, typename... Args> \
Expand Down
8 changes: 4 additions & 4 deletions be/src/exprs/hybrid_set.h
Original file line number Diff line number Diff line change
Expand Up @@ -222,27 +222,27 @@ class HybridSetBase : public RuntimeFilterFuncBase {

virtual void find_batch(const doris::vectorized::IColumn& column, size_t rows,
doris::vectorized::ColumnUInt8::Container& results) {
LOG(FATAL) << "HybridSetBase not support find_batch";
throw Exception(Status::FatalError("HybridSetBase not support find_batch"));
__builtin_unreachable();
}

virtual void find_batch_negative(const doris::vectorized::IColumn& column, size_t rows,
doris::vectorized::ColumnUInt8::Container& results) {
LOG(FATAL) << "HybridSetBase not support find_batch_negative";
throw Exception(Status::FatalError("HybridSetBase not support find_batch_negative"));
__builtin_unreachable();
}

virtual void find_batch_nullable(const doris::vectorized::IColumn& column, size_t rows,
const doris::vectorized::NullMap& null_map,
doris::vectorized::ColumnUInt8::Container& results) {
LOG(FATAL) << "HybridSetBase not support find_batch_nullable";
throw Exception(Status::FatalError("HybridSetBase not support find_batch_nullable"));
__builtin_unreachable();
}

virtual void find_batch_nullable_negative(const doris::vectorized::IColumn& column, size_t rows,
const doris::vectorized::NullMap& null_map,
doris::vectorized::ColumnUInt8::Container& results) {
LOG(FATAL) << "HybridSetBase not support find_batch_nullable_negative";
throw Exception(Status::FatalError("HybridSetBase not support find_batch_nullable_negative"));
__builtin_unreachable();
}

Expand Down
2 changes: 1 addition & 1 deletion be/src/gutil/port.h
Original file line number Diff line number Diff line change
Expand Up @@ -780,7 +780,7 @@ typedef int ssize_t;
using std::ostream;
inline ostream& operator<<(ostream& os, const unsigned __int64& num) {
// Fake operator; doesn't actually do anything.
LOG(FATAL) << "64-bit ostream operator << not supported in VC++ 6";
throw Exception(Statsu::FatalError("64-bit ostream operator << not supported in VC++ 6"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here's a typo?

return os;
}
#endif
Expand Down
23 changes: 13 additions & 10 deletions be/src/gutil/strings/escaping.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,15 @@
#include "gutil/strings/escaping.h"

#include <assert.h>
#include <glog/logging.h>
#include <stdio.h>
#include <string.h>
#include <glog/logging.h>

#include <limits>
#include <ostream>

#include "common/exception.h"

using std::numeric_limits;
#include <vector>

Expand All @@ -20,8 +23,8 @@ using std::vector;
#include "gutil/integral_types.h"
#include "gutil/port.h"
#include "gutil/stl_util.h"
#include "gutil/utf/utf.h" // for runetochar
#include "gutil/strings/strcat.h"
#include "gutil/utf/utf.h" // for runetochar

namespace strings {

Expand Down Expand Up @@ -143,9 +146,9 @@ int UnescapeCEscapeSequences(const char* source, char* dest, vector<string>* err
if (IS_OCTAL_DIGIT(p[1])) // safe (and easy) to do this twice
ch = ch * 8 + *++p - '0'; // now points at last digit
if (ch > 0xFF)
LOG_STRING(ERROR, errors) << "Value of "
<< "\\" << string(octal_start, p + 1 - octal_start)
<< " exceeds 8 bits";
LOG_STRING(ERROR, errors)
<< "Value of " << "\\" << string(octal_start, p + 1 - octal_start)
<< " exceeds 8 bits";
*d++ = ch;
break;
}
Expand All @@ -166,8 +169,8 @@ int UnescapeCEscapeSequences(const char* source, char* dest, vector<string>* err
ch = (ch << 4) + hex_digit_to_int(*++p);
if (ch > 0xFF)
LOG_STRING(ERROR, errors)
<< "Value of "
<< "\\" << string(hex_start, p + 1 - hex_start) << " exceeds 8 bits";
<< "Value of " << "\\" << string(hex_start, p + 1 - hex_start)
<< " exceeds 8 bits";
*d++ = ch;
break;
}
Expand Down Expand Up @@ -899,8 +902,7 @@ int Base64UnescapeInternal(const char* src, int szsrc, char* dest, int szdest,
// of data bytes that must remain in the input to avoid aborting the
// loop.
#define GET_INPUT(label, remain) \
label: \
--szsrc; \
label : --szsrc; \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont re-format it if you didn't change

ch = *src++; \
decode = unbase64[ch]; \
if (decode < 0) { \
Expand Down Expand Up @@ -1084,7 +1086,8 @@ int Base64UnescapeInternal(const char* src, int szsrc, char* dest, int szdest,

default:
// state should have no other values at this point.
LOG(FATAL) << "This can't happen; base64 decoder state = " << state;
throw doris::Exception(
doris::Status::FatalError("This can't happen; base64 decoder state = {}", state));
}

// The remainder of the string should be all whitespace, mixed with
Expand Down
28 changes: 17 additions & 11 deletions be/src/gutil/strings/numbers.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,18 @@
#include <ctype.h>
#include <errno.h>
#include <float.h> // for DBL_DIG and FLT_DIG
#include <math.h> // for HUGE_VAL
#include <inttypes.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: inclusion of deprecated C++ header 'inttypes.h'; consider using 'cinttypes' instead [modernize-deprecated-headers]

Suggested change
#include <inttypes.h>
#include <cinttypes>

#include <math.h> // for HUGE_VAL
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: inclusion of deprecated C++ header 'math.h'; consider using 'cmath' instead [modernize-deprecated-headers]

Suggested change
#include <math.h> // for HUGE_VAL
#include <cmath> // for HUGE_VAL

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <inttypes.h>
#include <sys/types.h>

#include <limits>
#include <ostream>

#include "common/exception.h"

using std::numeric_limits;
#include <string>

Expand All @@ -28,7 +31,6 @@ using std::string;
#include <fmt/format.h>

#include "common/logging.h"

#include "gutil/gscoped_ptr.h"
#include "gutil/integral_types.h"
#include "gutil/stringprintf.h"
Expand Down Expand Up @@ -702,12 +704,16 @@ size_t u64tostr_base36(uint64 number, size_t buf_size, char* buffer) {
}

// Generate functions that wrap safe_strtoXXX_base.
#define GEN_SAFE_STRTO(name, type) \
bool name##_base(const string& str, type* value, int base) { \
return name##_base(str.c_str(), value, base); \
} \
bool name(const char* str, type* value) { return name##_base(str, value, 10); } \
bool name(const string& str, type* value) { return name##_base(str.c_str(), value, 10); }
#define GEN_SAFE_STRTO(name, type) \
bool name##_base(const string& str, type* value, int base) { \
return name##_base(str.c_str(), value, base); \
} \
bool name(const char* str, type* value) { \
return name##_base(str, value, 10); \
} \
bool name(const string& str, type* value) { \
return name##_base(str.c_str(), value, 10); \
}
GEN_SAFE_STRTO(safe_strto32, int32);
GEN_SAFE_STRTO(safe_strtou32, uint32);
GEN_SAFE_STRTO(safe_strto64, int64);
Expand Down Expand Up @@ -772,8 +778,8 @@ uint64 atoi_kmgt(const char* s) {
scale = GG_ULONGLONG(1) << 40;
break;
default:
LOG(FATAL) << "Invalid mnemonic: `" << c << "';"
<< " should be one of `K', `M', `G', and `T'.";
throw doris::Exception(doris::Status::FatalError(
"Invalid mnemonic: `{}'; should be one of `K', `M', `G', and `T'.", c));
}
}
return n * scale;
Expand Down
8 changes: 5 additions & 3 deletions be/src/gutil/strings/util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,14 @@
#include <stdio.h>
#include <string.h>
#include <time.h> // for FastTimeToBuffer()

#include <algorithm>
#include <iterator>
#include <mutex>
#include <ostream>

#include "common/exception.h"

using std::copy;
using std::max;
using std::min;
Expand All @@ -33,13 +36,12 @@ using std::string;
using std::vector;

#include "common/logging.h"

#include "gutil/stl_util.h" // for string_as_array, STLAppendToString
#include "gutil/stringprintf.h"
#include "gutil/strings/ascii_ctype.h"
#include "gutil/strings/numbers.h"
#include "gutil/strings/stringpiece.h"
#include "gutil/utf/utf.h"
#include "gutil/stringprintf.h"

#ifdef OS_WINDOWS
#ifdef min // windows.h defines this to something silly
Expand Down Expand Up @@ -489,7 +491,7 @@ const char* strstr_delimited(const char* haystack, const char* needle, char deli
++haystack;
}
}
LOG(FATAL) << "Unreachable statement";
throw doris::Exception(doris::Status::FatalError("Unreachable statement"));
return nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this line

}

Expand Down
8 changes: 6 additions & 2 deletions be/src/gutil/threading/thread_collision_warner.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@

#include "gutil/threading/thread_collision_warner.h"

#include "common/exception.h"
#include "common/status.h"

#ifdef __linux__
#include <syscall.h>
#else
Expand All @@ -19,8 +22,9 @@
namespace base {

void DCheckAsserter::warn(int64_t previous_thread_id, int64_t current_thread_id) {
LOG(FATAL) << "Thread Collision! Previous thread id: " << previous_thread_id
<< ", current thread id: " << current_thread_id;
throw doris::Exception(doris::Status::FatalError(
"Thread Collision! Previous thread id: {}, current thread id: {}", previous_thread_id,
current_thread_id));
}

static subtle::Atomic64 CurrentThread() {
Expand Down
4 changes: 2 additions & 2 deletions be/src/io/file_factory.h
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,9 @@ class FileFactory {
case TStorageBackendType::HDFS:
return TFileType::FILE_HDFS;
default:
LOG(FATAL) << "not match type to convert, from type:" << type;
throw Exception(Status::FatalError("not match type to convert, from type:{}", type));
}
LOG(FATAL) << "__builtin_unreachable";
throw Exception(Status::FatalError("__builtin_unreachable"));
__builtin_unreachable();
}
};
Expand Down
Loading
Loading