Using exceptions instead of Status or OLAPStatus

[yiguolei]

There are 3 status code in doris: AgentStatus, OLAPStatus, Status and I am trying to unify them to Status. But I find there are too many rubbish code when using status code. For example:

1. There are about 1860 calls to RETURN_IF_ERROR macros. And about 184 calls to RETURN_NOT_OK.
2. Every method call should check the return status code and logging it if some thing errors. Sometimes the developer may forget to check it then bugs come in. Like this:

OLAPStatus Compaction::compact() {
    RETURN_NOT_OK(prepare_compact());
    RETURN_NOT_OK(execute_compact());
    return OLAP_SUCCESS;
}

and this:

void AgentServer::make_snapshot(TAgentResult& t_agent_result,
                                const TSnapshotRequest& snapshot_request) {
    Status ret_st;
    string snapshot_path;
    bool allow_incremental_clone = false;
    OLAPStatus err_code =
            SnapshotManager::instance()->make_snapshot(snapshot_request, &snapshot_path, &allow_incremental_clone);
    if (err_code != OLAP_SUCCESS) {
        LOG(WARNING) << "fail to make_snapshot. tablet_id=" << snapshot_request.tablet_id
                     << ", schema_hash=" << snapshot_request.schema_hash
                     << ", error_code=" << err_code;
        ret_st = Status::RuntimeError(
                strings::Substitute("fail to make_snapshot. err_code=$0", err_code));
    } else {
        LOG(INFO) << "success to make_snapshot. tablet_id=" << snapshot_request.tablet_id
                  << ", schema_hash=" << snapshot_request.schema_hash
                  << ", snapshot_path: " << snapshot_path;
        t_agent_result.__set_snapshot_path(snapshot_path);
        t_agent_result.__set_allow_incremental_clone(allow_incremental_clone);
    }

    ret_st.to_thrift(&t_agent_result.status);
    t_agent_result.__set_snapshot_version(snapshot_request.preferred_snapshot_version);
}

Most of these code is useless....

3. There are no call stacks if error happens. For example, if method A call method B, and method C also call method B, if B return an error status , we did not know which method call B, we have to do logging in both A and C and this will help us to find what happens but the developer need very familier with our logging...

The error message should contain 3 components:

1. Error Code, some application need this to do something.
2. Error Message, a simplified message for the end user.
3. Exception Stack, for the developer to know where bug happens.

I find maybe we could just use CPP Exception to simplify programming and error messages. And the caller method do not need to check the return value in most times.

[morningman]

Could you give some disadvantage of C++ exception?

[yiguolei]

1. C++ Exception has error stacks, we could find get the function stack when error happens and do not need to do logging for every function call.
2. C++ Exception will exist the function if the function not catch the exception and we do not need to check the return status for every function call.

[yangzhg]

Exceptions have a very large performance loss compared to Status, and try catch has roughly 10 times the performance loss of an if statement. There is a benchmark https://quick-bench.com/q/7GrS5_yZDwcouuCPX-oJoMh2b2k
So I think use exception is not a good idea, but it is necessary to unify OLAPStatus and Status

[yiguolei]

It only have performance loss when go to catch clause. But if our code do not go to catch clause, there is no performance loss and we have do some test in Tencent. You can refer https://mp.weixin.qq.com/s/P5FrnLsm74bi99UGUHjS7Q and we could also read the ABI https://itanium-cxx-abi.github.io/cxx-abi/abi-eh.html.
And clickhouse is using exception to deal with errors.

[wangbo]

just use CPP Exception to simplify programming and error messages
A bit abstract, is there are articles for reference.

[yiguolei]

For example, clickhouse will throw exception when error happens and the caller function will not deal with the errors and the code is more readable and maintainable.

void MergeTreeData::validateDetachedPartName(const String & name)
{
    if (name.find('/') != std::string::npos || name == "." || name == "..")
        throw DB::Exception("Invalid part name '" + name + "'", ErrorCodes::INCORRECT_FILE_NAME);

    if (startsWith(name, "attaching_") || startsWith(name, "deleting_"))
        throw DB::Exception("Cannot drop part " + name + ": "
                            "most likely it is used by another DROP or ATTACH query.",
                            ErrorCodes::BAD_DATA_PART_NAME);
}

In this case the function that call validateDetachedPartName does not need to check the return value .

[yiguolei]

More clearly, the code in my issue could be simplified like this

void AgentServer::make_snapshot(TAgentResult& t_agent_result,
                                const TSnapshotRequest& snapshot_request) {
    Status ret_st;
    string snapshot_path;
    bool allow_incremental_clone = false;
            SnapshotManager::instance()->make_snapshot(snapshot_request, &snapshot_path, &allow_incremental_clone);
        LOG(INFO) << "success to make_snapshot. tablet_id=" << snapshot_request.tablet_id
                  << ", schema_hash=" << snapshot_request.schema_hash
                  << ", snapshot_path: " << snapshot_path;
        t_agent_result.__set_snapshot_path(snapshot_path);
        t_agent_result.__set_allow_incremental_clone(allow_incremental_clone);
    t_agent_result.__set_snapshot_version(snapshot_request.preferred_snapshot_version);
}