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

linrrzqqq · 2024-10-23T10:59:45Z

Proposed changes

Issue Number: close #40835

use throw Exception to replace them which not in if constexpr， and change part of INTERNAL_ERROR in this pr(file aggregate_function_reader_first_last.h and aggregate_function_window.h) to FatalError.
for those in if constexpr else{...}, use static_assert about template argument which used in that judgement to advance them to compile time

but there seems to be some bugs with the template parameter instantiation in the files comparison_predicate.h, set_probe_sink_operator.cpp, set_sink_operator.cpp, in_list_predicate.h and set_source_operator.cpp that I haven't modified yet.

doris-robot · 2024-10-23T10:59:49Z

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR

Since 2024-03-18, the Document has been moved to doris-website.
See Doris Document.

github-actions · 2024-10-23T11:09:24Z

clang-tidy review says "All clean, LGTM! 👍"

zclllyybb · 2024-10-24T06:24:15Z

be/src/vec/functions/function_cast.h

@@ -651,7 +651,7 @@ struct ConvertImplNumberToJsonb {
            } else if constexpr (std::is_same_v<ColumnFloat64, ColumnType>) {
                writer.writeDouble(data[i]);
            } else {
-                LOG(FATAL) << "unsupported type ";
+                throw Exception(Status::NotSupported("unsupported type "));


for if constexpr, you can use static_assert rather than throw

zclllyybb · 2024-10-24T06:34:23Z

be/src/common/elf.cpp

@@ -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::IOError("failed to get file size {}: ({}), {}", _file.native(),


please add a new error type. we can call it FatalError. and for all Exception changed from LOG(FATAL), use it as the error code

github-actions · 2024-11-12T16:32:24Z

`sh-checker report`

To get the full details, please check in the job output.

shellcheck errors


'shellcheck ' returned error 1 finding the following syntactical issues:

----------

In build.sh line 297:
    local submodule_path=$1
    ^---------------------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 298:
    local submodule_name=$2
    ^---------------------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 299:
    local archive_url=$3
    ^------------------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 301:
    set +e
    ^----^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 302:
    cd "${DORIS_HOME}"
    ^----------------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 303:
    echo "Update ${submodule_name} submodule ..."
    ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 304:
    git submodule update --init --recursive "${submodule_path}"
    ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 305:
    exit_code=$?
    ^----------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 306:
    if [[ "${exit_code}" -eq 0 ]]; then
    ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).
       ^------------------------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 307:
        cd "${submodule_path}"
        ^--------------------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 308:
        submodule_commit_id=$(git rev-parse HEAD)
        ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).
                              ^----------------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 309:
        cd -
        ^--^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 310:
        expect_submodule_commit_id=$(git ls-tree HEAD "${submodule_path}" | awk '{print $3}')
        ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).
                                     ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 311:
        echo "Current commit ID of ${submodule_name} submodule: ${submodule_commit_id}, expected is ${expect_submodule_commit_id}"
        ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 313:
    set -e
    ^----^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 314:
    if [[ "${exit_code}" -ne 0 ]]; then
    ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).
       ^------------------------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 315:
        set +e
        ^----^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 317:
        submodule_commit=$(git ls-tree HEAD "${submodule_path}" | awk '{print $3}')
        ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).
                           ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 318:
        exit_code=$?
        ^----------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 319:
        if [[ "${exit_code}" = "0" ]]; then
        ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).
           ^------------------------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 320:
            commit_specific_url=$(echo "${archive_url}" | sed "s/refs\/heads/${submodule_commit}/")
            ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).
                                  ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 322:
            commit_specific_url="${archive_url}"
            ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 324:
        set -e
        ^----^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 325:
        echo "Update ${submodule_name} submodule failed, start to download and extract ${commit_specific_url}"
        ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 327:
        mkdir -p "${DORIS_HOME}/${submodule_path}"
        ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build.sh line 328:
        curl -L "${commit_specific_url}" | tar -xz -C "${DORIS_HOME}/${submodule_path}" --strip-components=1
        ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).

For more information:
  https://www.shellcheck.net/wiki/SC2317 -- Command appears to be unreachable...
----------

You can address the above issues in one of three ways:
1. Manually correct the issue in the offending shell script;
2. Disable specific issues by adding the comment:
  # shellcheck disable=NNNN
above the line that contains the issue, where NNNN is the error code;
3. Add '-e NNNN' to the SHELLCHECK_OPTS setting in your .yml action file.

shfmt errors

'shfmt ' found no issues.

zclllyybb · 2024-11-13T03:46:33Z

build.sh

@@ -551,8 +551,8 @@ FE_MODULES="$(

 # Clean and build Backend
 if [[ "${BUILD_BE}" -eq 1 ]]; then
-    update_submodule "be/src/apache-orc" "apache-orc" "https://github.com/apache/doris-thirdparty/archive/refs/heads/orc.tar.gz"
-    update_submodule "be/src/clucene" "clucene" "https://github.com/apache/doris-thirdparty/archive/refs/heads/clucene.tar.gz"
+    # update_submodule "be/src/apache-orc" "apache-orc" "https://github.com/apache/doris-thirdparty/archive/ads/clucene.tarefs/heads/orc.tar.gz"


don't commit this

github-actions

clang-tidy made some suggestions

github-actions · 2024-11-21T08:17:53Z

be/src/gutil/strings/numbers.cc

@@ -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>


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

Suggested change

#include <inttypes.h>

#include <cinttypes>

github-actions · 2024-11-21T08:17:53Z

be/src/gutil/strings/numbers.cc

@@ -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>
+#include <math.h> // for HUGE_VAL


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

zclllyybb · 2024-11-21T18:34:29Z

be/src/olap/comparison_predicate.h

@@ -524,7 +526,7 @@ class ComparisonPredicateBase : public ColumnPredicate {
                _base_loop_bit<is_nullable, is_and>(sel, size, flags, null_map, data_array,
                                                    dict_code);
            } else {
-                LOG(FATAL) << "column_dictionary must use StringRef predicate.";
+                static_assert(!std::is_same_v<T, StringRef>, "column_dictionary must use StringRef predicate.");


we wrote if constexpr (std::is_same_v<T, StringRef>) upon
so here going into else means T is not same with StringRef. we want a failure here. so I think we should assert std::is_same_v<T, StringRef>, right?

sorry I made a mistake here, and I have done fixing three similar errors in this file right away

zclllyybb · 2024-11-24T09:36:55Z

be/src/gutil/port.h

@@ -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"));


here's a typo?

zclllyybb · 2024-11-24T09:37:22Z

be/src/gutil/strings/escaping.cc

@@ -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;                                          \


dont re-format it if you didn't change

zclllyybb · 2024-11-24T09:38:51Z

be/src/common/exception.cpp

@@ -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(


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

zclllyybb

I think we should leave dwarf.cpp, elf.cpp and gutil/port.h as they are. dont patch this change on those filles.

zclllyybb · 2024-11-29T09:06:42Z

be/src/gutil/strings/util.cc

@@ -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;


remove this line

zclllyybb · 2024-11-29T09:07:45Z

be/src/olap/in_list_predicate.h

@@ -431,8 +431,8 @@ class InListPredicateBase : public ColumnPredicate {
                    }
                }
            } else {
-                LOG(FATAL) << "column_dictionary must use StringRef predicate.";
-                __builtin_unreachable();
+                throw Exception(


use static_assert. or just not modify this in this PR

zclllyybb · 2024-11-29T09:08:05Z

be/src/olap/key_coder.h

@@ -300,7 +300,7 @@ class KeyCoderTraits<FieldType::OLAP_FIELD_TYPE_CHAR> {
    }

    static Status decode_ascending(Slice* encoded_key, size_t index_size, uint8_t* cell_ptr) {
-        LOG(FATAL) << "decode_ascending is not implemented";
+        throw Exception(Status::FatalError("decode_ascending is not implemented"));
        return Status::OK();


remove the returns

linrrzqqq · 2024-12-12T12:08:29Z

run buildall

linrrzqqq · 2024-12-12T13:43:18Z

run buildall

github-actions · 2024-12-12T13:46:17Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-12-12T18:00:38Z

clang-tidy review says "All clean, LGTM! 👍"

linrrzqqq · 2024-12-12T18:01:08Z

run buildall

linrrzqqq · 2024-12-13T00:47:01Z

run buildall

github-actions · 2024-12-13T00:47:47Z

clang-tidy review says "All clean, LGTM! 👍"

linrrzqqq · 2024-12-17T08:15:54Z

run buildall

github-actions · 2024-12-17T08:23:44Z

clang-tidy review says "All clean, LGTM! 👍"

linrrzqqq · 2024-12-17T14:29:22Z

run buildall

github-actions · 2024-12-17T14:37:53Z

clang-tidy review says "All clean, LGTM! 👍"

doris-robot · 2024-12-17T16:22:20Z

TeamCity be ut coverage result:
Function Coverage: 38.87% (10129/26061)
Line Coverage: 29.78% (85138/285905)
Region Coverage: 28.88% (43419/150344)
Branch Coverage: 25.40% (22124/87104)
Coverage Report: http://coverage.selectdb-in.cc/coverage/ac1d12e062d3d12ecf38fb94920d748485f03083_ac1d12e062d3d12ecf38fb94920d748485f03083/report/index.html

linrrzqqq · 2024-12-18T02:51:24Z

run p0

linrrzqqq · 2024-12-18T06:14:09Z

run p0

zclllyybb

LGTM

github-actions · 2024-12-18T07:11:45Z

PR approved by anyone and no changes requested.

xinyiZzz

LGTM

github-actions · 2024-12-19T03:13:02Z

PR approved by at least one committer and no changes requested.

…42344)" This reverts commit 55c26e0.

1

3453eb0

zclllyybb reviewed Oct 24, 2024

View reviewed changes

Replace

ae66093

zclllyybb reviewed Nov 13, 2024

View reviewed changes

2

afeb18b

github-actions bot reviewed Nov 21, 2024

View reviewed changes

Remove unreachable

9b0339c

zclllyybb reviewed Nov 21, 2024

View reviewed changes

zclllyybb reviewed Nov 24, 2024

View reviewed changes

zclllyybb suggested changes Nov 24, 2024

View reviewed changes

Excepte assert

aaed254

linrrzqqq marked this pull request as ready for review November 28, 2024 16:30

linrrzqqq requested review from platoneko, gavinchou and dataroaring as code owners November 28, 2024 16:30

linrrzqqq added 5 commits November 29, 2024 00:36

Final

d622461

ralfinal

9290081

format

e8333c3

format2

8dfe7b6

format3

b5fa54b

linrrzqqq changed the title ~~[Enhancement](Log) Reduce usage of log fatal~~ [Enhancement](Log) Reduce usage of log fatal(PART I) Nov 29, 2024

zclllyybb reviewed Nov 29, 2024

View reviewed changes

8

cc18a04

rmv msg

f704fb1

9

da97163

rmv }

e5dfd18

wtd

ac1d12e

zclllyybb approved these changes Dec 18, 2024

View reviewed changes

github-actions bot added the reviewed label Dec 18, 2024

xinyiZzz approved these changes Dec 19, 2024

View reviewed changes

github-actions bot added the approved Indicates a PR has been approved by one committer. label Dec 19, 2024

gavinchou approved these changes Dec 19, 2024

View reviewed changes

BiteTheDDDDt merged commit 55c26e0 into apache:master Dec 19, 2024
28 of 30 checks passed

wangbo added a commit to wangbo/incubator-doris that referenced this pull request Dec 19, 2024

Revert "[Enhancement](Log) Reduce usage of log fatal(PART I) (apache#…

1d937b2

…42344)" This reverts commit 55c26e0.

zclllyybb mentioned this pull request Dec 19, 2024

[fix](ub) Dont throw in noexcept function #45672

Merged

16 tasks

linrrzqqq deleted the log_fatal branch January 18, 2025 09:17

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

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

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

Conversation

linrrzqqq commented Oct 23, 2024 • edited Loading

Proposed changes

doris-robot commented Oct 23, 2024

github-actions bot commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 12, 2024

sh-checker report

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Nov 21, 2024

Choose a reason for hiding this comment

github-actions bot Nov 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zclllyybb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linrrzqqq commented Dec 12, 2024

linrrzqqq commented Dec 12, 2024

github-actions bot commented Dec 12, 2024

github-actions bot commented Dec 12, 2024

linrrzqqq commented Dec 12, 2024

linrrzqqq commented Dec 13, 2024

github-actions bot commented Dec 13, 2024

linrrzqqq commented Dec 17, 2024

github-actions bot commented Dec 17, 2024

linrrzqqq commented Dec 17, 2024

github-actions bot commented Dec 17, 2024

doris-robot commented Dec 17, 2024

linrrzqqq commented Dec 18, 2024

linrrzqqq commented Dec 18, 2024

zclllyybb left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 18, 2024

xinyiZzz left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 19, 2024

linrrzqqq commented Oct 23, 2024 •

edited

Loading

`sh-checker report`