[Refactor] Change ALL OLAPStatus to Status #8855

yiguolei · 2022-04-06T08:02:36Z

Currently, there are 2 status code in BE, one is common/Status.h, and the other is olap/olap_define.h called OLAPStatus.
OLAPStatus is just an enum type, it is very simple and could not save many informations, I will unify these code to common/Status.

Proposed changes

Issue Number: close #xxx

Problem Summary:

Describe the overview of changes.

Checklist(Required)

Does it affect the original behavior: (Yes/No/I Don't know)
Has unit tests been added: (Yes/No/No Need)
Has document been added or modified: (Yes/No/No Need)
Does it need to update dependencies: (Yes/No)
Are there any changes that cannot be rolled back: (Yes/No)

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

jackwener

TL;DR

morningman · 2022-04-07T08:12:21Z

We should separate this PR

caiconghui · 2022-04-11T04:27:36Z

be/CMakeLists.txt

 # Set boost/stacktrace use backtrace api to unwind
 add_definitions(-DBOOST_STACKTRACE_USE_BACKTRACE)
-add_definitions(-DPRINT_ALL_ERR_STATUS_STACKTRACE)
+#add_definitions(-DPRINT_ALL_ERR_STATUS_STACKTRACE)


why comment out this config？

If this marco is enabled, BE will print all error status to log, it may cause performance problem. So that I disabled it by default. Should enable it manually if it is needed.

caiconghui · 2022-04-11T04:33:18Z

be/src/agent/agent_server.cpp

+    Status err_code =
            SnapshotManager::instance()->make_snapshot(snapshot_request, &snapshot_path, &allow_incremental_clone);
-    if (err_code != OLAP_SUCCESS) {
+    if (!err_code) {


why not err_code.ok()? !err_code is same with !err_code.ok() ??

I think maybe ret_code is better.
err_code is not a good name.It seems like !error_code means no error.

I think we should not modify variable name in this PR. Because there maybe multiple status variable in same function. Modify the variable name may cause logic bug. And also, there are too many variables, it may take lot of time.
We may refactor the status variable name in other PRs.

be/src/common/status.cpp

morningman · 2022-04-11T15:17:15Z

be/src/http/action/compaction_action.cpp

-            if (res == OLAP_ERR_BE_NO_SUITABLE_VERSION) {
+        res = base_compaction.compact();
+        if (!res) {
+            if (res == Status::OLAPInternalError(OLAP_ERR_BE_NO_SUITABLE_VERSION)) {


Here you call Status::OLAPInternalError only for checking the error code?
Why not do thing like if (res.err_code == OLAP_ERR_BE_NO_SUITABLE_VERSION)

Yes. We'd better not change logic here. And there are many code like this because I use search and replace to change the code. Maybe we could use some code like isNoSuitableVersionError here to check it.

be/src/olap/delete_handler.cpp

morningman · 2022-04-13T09:20:02Z

be/src/olap/memtable_flush_executor.cpp

+    Status s = memtable->flush();
+    LOG(WARNING) << "Flush memtable failed with res = " << s;
+    // If s is not ok, ignore the code, just use other code is ok
+    _flush_status.store(s.ok() ? OLAP_SUCCESS : OLAP_ERR_OTHER_ERROR);


Why not using the code from s?

Error code in S is int16_t, it may not be converted to ErrorCode. And I find we did not justify the errorcode in other code. So that I print the error stack in LOG(WARNING) and using OTHER_ERROR here.

be/src/olap/memtable_flush_executor.cpp

morningman · 2022-04-13T09:40:10Z

be/src/olap/rowset/CMakeLists.txt

    beta_rowset_reader.cpp
    beta_rowset_writer.cpp)
+
+target_compile_options(Rowset PUBLIC "-Wno-error=maybe-uninitialized")


Why adding this?

Some variables in rowset is not initialized, so that I has to add this option to compile successfully. I will add another PR to init all variables and remove this option.

be/src/olap/task/engine_batch_load_task.cpp

be/src/vec/exec/volap_scanner.cpp

Co-authored-by: Mingyu Chen <morningman.cmy@gmail.com>

wangbo · 2022-04-13T12:40:02Z

be/src/olap/rowset/alpha_rowset.cpp

-    if (largest_segment_group->find_short_key(end_key, &helper_cursor, false, &end_pos) !=
-        OLAP_SUCCESS) {
-        if (largest_segment_group->find_last_row_block(&end_pos) != OLAP_SUCCESS) {
+    if (!largest_segment_group->find_short_key(end_key, &helper_cursor, false, &end_pos)) {


Suggested change

if (!largest_segment_group->find_short_key(end_key, &helper_cursor, false, &end_pos)) {

if (largest_segment_group->find_short_key(end_key, &helper_cursor, false, &end_pos) != Status::OK()) {

wangbo · 2022-04-13T12:40:32Z

be/src/olap/rowset/alpha_rowset.cpp

-        OLAP_SUCCESS) {
-        if (largest_segment_group->find_last_row_block(&end_pos) != OLAP_SUCCESS) {
+    if (!largest_segment_group->find_short_key(end_key, &helper_cursor, false, &end_pos)) {
+        if (!largest_segment_group->find_last_row_block(&end_pos)) {


Suggested change

if (!largest_segment_group->find_last_row_block(&end_pos)) {

if (largest_segment_group->find_last_row_block(&end_pos) != Status::OK()) {

wangbo · 2022-04-13T12:51:33Z

be/src/olap/rowset/bit_field_reader.cpp


    if (_byte_reader->has_next()) {
-        if (OLAP_SUCCESS != (res = _byte_reader->next(&_current))) {
+        if (!(res = _byte_reader->next(&_current))) {


Suggested change

if (!(res = _byte_reader->next(&_current))) {

if (_byte_reader->next(&_current) != Status::OK()) {

I suggest we do not modify logic here, it will make the refactor work very difficult and may import bugs. For example, in this case, the res value maybe used in the latter code.

wangbo · 2022-04-13T12:52:14Z

be/src/olap/rowset/bit_field_reader.cpp


    if (0 == _bits_left) {
-        if (OLAP_SUCCESS != (res = _read_byte())) {
+        if (!(res = _read_byte())) {


Suggested change

if (!(res = _read_byte())) {

if (_read_byte() != res) {

wangbo · 2022-04-13T12:53:54Z

be/src/olap/rowset/bit_field_reader.cpp

+    Status res = Status::OK();

-    if (OLAP_SUCCESS != (res = _byte_reader->seek(position))) {
+    if (!(res = _byte_reader->seek(position))) {


Suggested change

if (!(res = _byte_reader->seek(position))) {

if (_byte_reader->seek(position) != res) {

wangbo · 2022-04-13T12:58:57Z

be/src/olap/rowset/column_data.cpp

-        if (OLAP_SUCCESS != res) {
-            OLAP_LOG_WARNING("fail to init segment reader. [res=%d]", res);
+        if (!res.ok()) {
+            LOG(WARNING) << "fail to init segment reader. res = " << res;


Why not using OLAP_LOG_WARNING?

OLAP_LOG_XXXX will be replaced with LOG(WARNING) in the future. LOGGING SYSTEM should also be uniformed.

caiconghui

the logic LGTM, but we need to keep one check status style in the future.

github-actions · 2022-04-13T14:40:55Z

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

github-actions · 2022-04-13T14:40:57Z

PR approved by anyone and no changes requested.

yiguolei · 2022-04-14T01:19:53Z

Add some performance test:

class Status {

public:

Status() {
_length = 0;
_precise_code = 0;
}

Status(int i) {
_precise_code = i % 17;
}

static Status OK() {
return Status();
}

static Status ERROR(int i) {
return Status(i);
}

bool operator == (const Status& st) { return _precise_code == st._precise_code; }

union {
char _state[2048];

  struct {
      // Message length == HEADER(7 bytes) + message size
      // Sometimes error message is empty, so that we could not use length==0 to indicate
      // whether there is error happens
      int64_t _length : 32;      
      int64_t _code : 8;          
      int64_t _precise_code : 16;
      int64_t _message : 8;       // save message since here
  };

};

static void newstatus(benchmark::State& state) {
// Code inside this loop is measured repeatedly
bool result = false;
int i = 0;
for (auto _ : state) {
Status status(i - 1);
++i;
if (status == Status::ERROR(i)) {
result = true;
} else {
result = false;
}
}
}
// Register the function as a benchmark
BENCHMARK(newstatus);

static void olapstatus(benchmark::State& state) {
// Code before the loop is not measured
bool result = false;
int i = 0;
for (auto _ : state) {
//Status status(i - 1);
++i;
if (i == 16) {
result = true;
} else {
result = false;
}
}
}
BENCHMARK(olapstatus);

new status is slower than olapstatus about 3.6%.

Since there are lot of other work during query and the status is returned batch by batch, so that the impact could be ignored.

yiguolei · 2022-04-14T01:23:07Z

Add SSB benchmark:
Environment:

ONE BE, ONE FE
CREATE LINEORDER TABLE with bucket num == 1, lineorder table with 600037902 rows.
set vectoried_engine = true;
set scanner thread == 1;
using select count(lo_shipmode) from lineorder; to test

new status:

old olapstatus:

It is very curious new status is a litter better than olapstatus.

morningman · 2022-04-14T03:33:22Z

I tested with jmeter, 50 concurrency, 1 FE, 1 BE
select * from tbl1 where k1=1 limit 1

The avg qps decreased from 1400/s to 1300/s, about 7% performance impact at high concurrency senario.
So I think it deserved to improve it.

But I think this PR can be merged first, and improve it later

1. The compile bug is introduced from #8855 2. FE ut bug is introduced from #8848 and #8770

Currently, there are 2 status code in BE, one is common/Status.h, and the other is olap/olap_define.h called OLAPStatus. OLAPStatus is just an enum type, it is very simple and could not save many informations, I will unify these code to common/Status.

1. The compile bug is introduced from apache#8855 2. FE ut bug is introduced from apache#8848 and apache#8770

Currently, there are 2 status code in BE, one is common/Status.h, and the other is olap/olap_define.h called OLAPStatus. OLAPStatus is just an enum type, it is very simple and could not save many informations, I will unify these code to common/Status.

1. The compile bug is introduced from apache#8855 2. FE ut bug is introduced from apache#8848 and apache#8770

Currently, there are 2 status code in BE, one is common/Status.h, and the other is olap/olap_define.h called OLAPStatus. OLAPStatus is just an enum type, it is very simple and could not save many informations, I will unify these code to common/Status.

1. The compile bug is introduced from apache#8855 2. FE ut bug is introduced from apache#8848 and apache#8770

yiguolei changed the title ~~[Refactor] remove OLAPStatus to Status~~ [Refactor] Change ALL OLAPStatus to Status Apr 6, 2022

jackwener previously approved these changes Apr 7, 2022

View reviewed changes

jackwener added the kind/refactor Issues or PRs to refactor code label Apr 7, 2022

yiguolei dismissed jackwener’s stale review via 85146bc April 7, 2022 02:43

morningman added the dev/backlog waiting to be merged in future dev branch label Apr 7, 2022

yiguolei force-pushed the remove_olap_status branch 2 times, most recently from e70fdbe to c513b19 Compare April 10, 2022 13:02

caiconghui reviewed Apr 11, 2022

View reviewed changes

yiguolei force-pushed the remove_olap_status branch from c513b19 to 54bbc8b Compare April 13, 2022 03:06

github-actions bot added the area/vectorization label Apr 13, 2022

ygl and others added 3 commits April 13, 2022 11:40

Uniform Status, using Status instead of OLAPStatus

079c321

fix test bug

984fd10

fix test bug

df40099

yiguolei force-pushed the remove_olap_status branch from 51bd540 to df40099 Compare April 13, 2022 03:40

Doris-Extras added 3 commits April 13, 2022 11:42

fix test bug

282695b

add more log info during flush

b6c422b

fix bug: res is not readable

3b2e33b

caiconghui reviewed Apr 13, 2022

View reviewed changes

be/src/common/status.cpp Show resolved Hide resolved

morningman reviewed Apr 13, 2022

View reviewed changes

yiguolei and others added 2 commits April 13, 2022 19:40

Update be/src/olap/task/engine_batch_load_task.cpp

371c744

Co-authored-by: Mingyu Chen <morningman.cmy@gmail.com>

fix bugs according to reviews

26e5336

wangbo reviewed Apr 13, 2022

View reviewed changes

caiconghui approved these changes Apr 13, 2022

View reviewed changes

github-actions bot added the approved Indicates a PR has been approved by one committer. label Apr 13, 2022

github-actions bot added the reviewed label Apr 13, 2022

morningman merged commit e5e0dc4 into apache:master Apr 14, 2022

morningman mentioned this pull request Apr 14, 2022

[fix](ut)(compile) Fix BE compile bug and FE unit test #9027

Merged

morningman added a commit that referenced this pull request Apr 14, 2022

[fix](ut)(compile) Fix BE compile bug and FE unit test (#9027)

579aee1

1. The compile bug is introduced from #8855 2. FE ut bug is introduced from #8848 and #8770

weizhengte pushed a commit to weizhengte/incubator-doris that referenced this pull request Apr 22, 2022

[fix](ut)(compile) Fix BE compile bug and FE unit test (apache#9027)

4e2bb62

1. The compile bug is introduced from apache#8855 2. FE ut bug is introduced from apache#8848 and apache#8770

zhengshiJ pushed a commit to zhengshiJ/incubator-doris that referenced this pull request Apr 27, 2022

[fix](ut)(compile) Fix BE compile bug and FE unit test (apache#9027)

9770ec9

1. The compile bug is introduced from apache#8855 2. FE ut bug is introduced from apache#8848 and apache#8770

englefly pushed a commit to englefly/incubator-doris that referenced this pull request May 23, 2022

[fix](ut)(compile) Fix BE compile bug and FE unit test (apache#9027)

3d8fb37

1. The compile bug is introduced from apache#8855 2. FE ut bug is introduced from apache#8848 and apache#8770

morningman mentioned this pull request Nov 21, 2022

Release Note 1.2.0 #14461

Closed

yiguolei deleted the remove_olap_status branch March 30, 2023 10:16

	if (!largest_segment_group->find_short_key(end_key, &helper_cursor, false, &end_pos)) {
	if (largest_segment_group->find_short_key(end_key, &helper_cursor, false, &end_pos) != Status::OK()) {

	if (!largest_segment_group->find_last_row_block(&end_pos)) {
	if (largest_segment_group->find_last_row_block(&end_pos) != Status::OK()) {

	if (!(res = _byte_reader->next(&_current))) {
	if (_byte_reader->next(&_current) != Status::OK()) {

	if (!(res = _byte_reader->seek(position))) {
	if (_byte_reader->seek(position) != res) {

[Refactor] Change ALL OLAPStatus to Status #8855

[Refactor] Change ALL OLAPStatus to Status #8855

Uh oh!

Conversation

yiguolei commented Apr 6, 2022

Proposed changes

Problem Summary:

Checklist(Required)

Further comments

Uh oh!

jackwener left a comment

Choose a reason for hiding this comment

Uh oh!

morningman commented Apr 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yiguolei Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caiconghui left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 13, 2022

Uh oh!

github-actions bot commented Apr 13, 2022

Uh oh!

yiguolei commented Apr 14, 2022

Uh oh!

yiguolei commented Apr 14, 2022

Uh oh!

morningman commented Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

yiguolei Apr 14, 2022 •

edited

Loading

morningman commented Apr 14, 2022 •

edited

Loading